Comment on attachment 769120 [details] [diff] [review] move HOST_CPPSRCS to mozbuild (logic) Add a passthrough conversion for HOST_CPPSRCS

Comment on attachment 769120 [details] [diff] [review] move HOST_CPPSRCS to mozbuild (logic) ping on code review

Conversion patch #1, try results on the way

Comment on attachment 770277 [details] [diff] [review] move HOST_CPPSRCS to mozbuild (file batch #1) >diff --git a/layout/style/test/Makefile.in b/layout/style/test/Makefile.in >--- a/layout/style/test/Makefile.in >+++ b/layout/style/test/Makefile.in >@@ -6,21 +6,21 @@ > DEPTH = @DEPTH@ > topsrcdir = @top_srcdir@ > srcdir = @srcdir@ > VPATH = @srcdir@ > relativesrcdir = @relativesrcdir@ > > include $(DEPTH)/config/autoconf.mk > >-HOST_CPPSRCS = \ >+DISABLED_HOST_CPPSRCS = \ > ListCSSProperties.cpp \ > $(NULL) > >-HOST_SIMPLE_PROGRAMS = $(addprefix host_, $(HOST_CPPSRCS:.cpp=$(HOST_BIN_SUFFIX))) >+HOST_SIMPLE_PROGRAMS = $(addprefix host_, $(DISABLED_HOST_CPPSRCS:.cpp=$(HOST_BIN_SUFFIX))) Can we just move this now to: HOST_SIMPLE_PROGRAMS = host_ListCSSProperties$(HOST_BIN_SUFFIX) It will make removing the DISABLED variable more straightforward later, and is more explicit. Everything looks good, so if try is happy I think it's ready to go.

Comment on attachment 770277 [details] [diff] [review] move HOST_CPPSRCS to mozbuild (file batch #1) Review of attachment 770277 [details] [diff] [review]: ----------------------------------------------------------------- Please stop doing this DISABLED_ prefixing. It just causes confusion and prevents removal of Makefile.in files. Please don't introduce dead code in an already complex component of our tree. You previously cited patch conflict as a rationale for wanting this. If Mercurial and/or Git's patch conflict tools don't work for you, I recommend the wiggle tool (http://linux.die.net/man/1/wiggle). It works quite well at resolving contextual conflicts due to removed blocks. I've used it extensively for moz.build conversions.

Comment on attachment 769120 [details] [diff] [review] move HOST_CPPSRCS to mozbuild (logic) Review of attachment 769120 [details] [diff] [review]: ----------------------------------------------------------------- I'd prefer HOST_CPP_SOURCES, but I'm kinda in a honeybadger mood right now and precedent is already there in the form of HOST_CSRCS. We'll fix all this later with the revised binary declaration grammar.

(In reply to Gregory Szorc [:gps] from comment #6) > Comment on attachment 770277 [details] [diff] [review] > move HOST_CPPSRCS to mozbuild (file batch #1) > > Review of attachment 770277 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please stop doing this DISABLED_ prefixing. It just causes confusion and > prevents removal of Makefile.in files. Please don't introduce dead code in > an already complex component of our tree. Why is Makefile.in being removed before we are sure the mozbuild patch has landed and stuck ? One of the conversion patches for SIMPLE_PROGRAMS was backed out recently because of a problem on windows which implies that conversion is not yet finished. Also if Makefile.in has been removed and mozbuild rules are backed out we will very likely stop building the target(s) altogether. Anyway these are removed after a week or so if the patch is stable so not sure why temporary renaming of a variable is causing such a horrendous problem. > You previously cited patch conflict as a rationale for wanting this. If > Mercurial and/or Git's patch conflict tools don't work for you, I recommend > the wiggle tool (http://linux.die.net/man/1/wiggle). It works quite well at > resolving contextual conflicts due to removed blocks. I've used it > extensively for moz.build conversions. A quick text diff to show content changes {only for convesrions} is a lot faster than having to fire up a graphic merge find deltas then merge them into the 3rd file.

Same patch, strip DISABLED_ tokens from Makefile.in. Just work w/o a saftey net.

Comment on attachment 770817 [details] [diff] [review] move HOST_CPPCSRCS to mozbuild >diff --git a/layout/style/test/Makefile.in b/layout/style/test/Makefile.in >--- a/layout/style/test/Makefile.in >+++ b/layout/style/test/Makefile.in >@@ -6,21 +6,18 @@ > DEPTH = @DEPTH@ > topsrcdir = @top_srcdir@ > srcdir = @srcdir@ > VPATH = @srcdir@ > relativesrcdir = @relativesrcdir@ > > include $(DEPTH)/config/autoconf.mk > >-HOST_CPPSRCS = \ >- ListCSSProperties.cpp \ >- $(NULL) > >-HOST_SIMPLE_PROGRAMS = $(addprefix host_, $(HOST_CPPSRCS:.cpp=$(HOST_BIN_SUFFIX))) >+HOST_SIMPLE_PROGRAMS = $(addprefix host_, $(DISABLED_HOST_CPPSRCS:.cpp=$(HOST_BIN_SUFFIX))) This still needs to be fixed. I suggest doing this (please double check I'm naming it properly) - HOST_SIMPLE_PROGRAMS = host_ListCSSProperties$(HOST_BIN_SUFFIX) >diff --git a/toolkit/crashreporter/google-breakpad/src/common/solaris/Makefile.in b/toolkit/crashreporter/google-breakpad/src/common/solaris/Makefile.in >--- a/toolkit/crashreporter/google-breakpad/src/common/solaris/Makefile.in >+++ b/toolkit/crashreporter/google-breakpad/src/common/solaris/Makefile.in >@@ -9,18 +9,13 @@ VPATH = @srcdir@ > > include $(DEPTH)/config/autoconf.mk > > > LOCAL_INCLUDES = -I$(srcdir)/../.. > > # not compiling http_upload.cc currently > # since it depends on libcurl nit: Mind as well remove this comment from the Makefile, since it is now in the moz.build file.

Comment on attachment 770817 [details] [diff] [review] move HOST_CPPCSRCS to mozbuild Cancelling review because of prior r-.

Inlined host_ListCSSProperties assignment. Cleanup script will only remove var assignments so this slipped through. Also purged comment from toolkit/crashreporter.

Comment on attachment 772254 [details] [diff] [review] move HOST_CPPCSRCS to mozbuild (file batch #1) http://tbpl.mozilla.org/?tree=Try&rev=275301b8023c try failure reported on panda and unagi

Comment on attachment 772254 [details] [diff] [review] move HOST_CPPCSRCS to mozbuild (file batch #1) Looks good!

(In reply to Joey Armstrong [:joey] from comment #14) > Comment on attachment 772254 [details] [diff] [review] > move HOST_CPPCSRCS to mozbuild (file batch #1) > > http://tbpl.mozilla.org/?tree=Try&rev=275301b8023c > try failure reported on panda and unagi media/libpng/arm/arm_init.c not appended to CSRCS in Makefile.in. Kyle thought this may be the reason for the upgrade patch being backed out on the 2nd.

Second batch of converted files with status from a try build. http://tbpl.mozilla.org/?tree=Try&rev=f89547ca5a7e

Comment on attachment 802330 [details] [diff] [review] move HOST_CPPSRCS to mozbuild (batch #2) >diff --git a/layout/style/test/moz.build b/layout/style/test/moz.build >--- a/layout/style/test/moz.build >+++ b/layout/style/test/moz.build >@@ -3,8 +3,12 @@ > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > TEST_DIRS += ['chrome'] > > MODULE = 'layout' > >+HOST_CPPSRCS += [ >+ 'ListCSSProperties.cpp', >+] >+ nit: The last empty line here has 4 blank spaces. Everything else looks good to me!

(In reply to Michael Shal [:mshal] from comment #19) > Comment on attachment 802330 [details] [diff] [review] > move HOST_CPPSRCS to mozbuild (batch #2) > > >diff --git a/layout/style/test/moz.build b/layout/style/test/moz.build > >--- a/layout/style/test/moz.build > >+++ b/layout/style/test/moz.build > >@@ -3,8 +3,12 @@ > > # This Source Code Form is subject to the terms of the Mozilla Public > > # License, v. 2.0. If a copy of the MPL was not distributed with this > > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > > > TEST_DIRS += ['chrome'] > > > > MODULE = 'layout' > > > >+HOST_CPPSRCS += [ > >+ 'ListCSSProperties.cpp', > >+] > >+ > > nit: The last empty line here has 4 blank spaces. > > Everything else looks good to me! If we could just delet all this it would look even better :). Thanks

Comment on attachment 803163 [details] [diff] [review] move HOST_CPPSRCS to mozbuild. Prune trailing whitespace on layout/style/test/moz.build, r=mshal carried forward

(In reply to Joey Armstrong [:joey] from comment #22) > Comment on attachment 803163 [details] [diff] [review] > move HOST_CPPSRCS to mozbuild. > > Prune trailing whitespace on layout/style/test/moz.build, r=mshal carried > forward try-test job: http://tbpl.mozilla.org/?tree=Try&rev=f452a68fb707

Last two stragglers converted for this variable.

Comment on attachment 803215 [details] [diff] [review] Move HOST_CPPSRCS to mozbuild (batch #4) try-build: http://tbpl.mozilla.org/?tree=Try&rev=3fc7dde18e4e

Comment on attachment 803163 [details] [diff] [review] move HOST_CPPSRCS to mozbuild. inbound push: http://hg.mozilla.org/integration/mozilla-inbound/rev/2f41b8eb22b5

Comment on attachment 803215 [details] [diff] [review] Move HOST_CPPSRCS to mozbuild (batch #4) >diff --git a/toolkit/crashreporter/google-breakpad/src/tools/solaris/dump_syms/moz.build b/toolkit/crashreporter/google-breakpad/src/tools/solaris/dump_syms/moz.build >--- a/toolkit/crashreporter/google-breakpad/src/tools/solaris/dump_syms/moz.build >+++ b/toolkit/crashreporter/google-breakpad/src/tools/solaris/dump_syms/moz.build >@@ -1,6 +1,10 @@ > # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*- > # vim: set filetype=python: > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > >+HOST_CPPSRCS += [ >+ 'dump_syms.cc', >+] >+ nit: More random space characters on the last line :) (git shows them with a red background, otherwise I probably wouldn't notice)

Two inbound pushes are in flight/need to land for this bug to be closed. Inbound push for attachment 803901 [details] [diff] [review]: http://hg.mozilla.org/integration/mozilla-inbound/rev/5063ebfa5609 inbound push for 803163: http://hg.mozilla.org/integration/mozilla-inbound/rev/2f41b8eb22b5