Closed Bug 888009 Opened 7 years ago Closed 6 years ago

move HOST_CPPSRCS to mozbuild

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: joey, Assigned: joey)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 7 obsolete files)

No description provided.
Blocks: nomakefiles
Assignee: nobody → jarmstrong
Comment on attachment 769120 [details] [diff] [review]
move HOST_CPPSRCS to mozbuild (logic)

Add a passthrough conversion for HOST_CPPSRCS
Attachment #769120 - Flags: review?(gps)
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
Attachment #770277 - Flags: review?(mshal)
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.
Attachment #770277 - Flags: review?(mshal) → review+
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.
Attachment #770277 - Flags: review-
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.
Attachment #769120 - Flags: review?(gps) → review+
(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.
Attached patch move HOST_CPPCSRCS to mozbuild (obsolete) — Splinter Review
Same patch, strip DISABLED_ tokens from Makefile.in.  Just work w/o a saftey net.
Attachment #770277 - Attachment is obsolete: true
Attachment #770817 - Flags: review?(mshal)
Attachment #770817 - Flags: review?(gps)
https://hg.mozilla.org/mozilla-central/rev/887ae544b828
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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.
Attachment #770817 - Flags: review?(mshal) → review-
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
Status: REOPENED → ASSIGNED
Comment on attachment 770817 [details] [diff] [review]
move HOST_CPPCSRCS to mozbuild

Cancelling review because of prior r-.
Attachment #770817 - Flags: review?(gps)
Inlined host_ListCSSProperties assignment.  Cleanup script will only remove var assignments so this slipped through.

Also purged comment from toolkit/crashreporter.
Attachment #770817 - Attachment is obsolete: true
Attachment #772254 - Flags: review?(mshal)
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!
Attachment #772254 - Flags: review?(mshal) → review+
(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.
Attached patch move HOST_CPPSRCS to mozbuild (obsolete) — Splinter Review
Assignee: jarmstrong → joey
Second batch of converted files with status from a try build.
http://tbpl.mozilla.org/?tree=Try&rev=f89547ca5a7e
Attachment #797369 - Attachment is obsolete: true
Attachment #802330 - Flags: review?(mshal)
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!
Attachment #802330 - Flags: review?(mshal) → review+
(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
Attachment #802330 - Attachment is obsolete: true
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
Attached patch 3-1: (obsolete) — Splinter Review
Last two stragglers converted for this variable.
Attachment #803213 - Attachment is obsolete: true
Attachment #803215 - Flags: review?(mshal)
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 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)
Attachment #803215 - Flags: review?(mshal) → review+
Attachment #803215 - Attachment is obsolete: true
Attachment #803900 - Attachment is obsolete: true
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
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/2f41b8eb22b5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.