Closed Bug 973649 Opened 10 years ago Closed 10 years ago

Add logic for CFLAGS, CXXFLAGS and LDFLAGS to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ehsan.akhgari, Assigned: Ms2ger)

References

Details

Attachments

(2 files)

      No description provided.
Assignee: nobody → ehsan
Comment on attachment 8377190 [details] [diff] [review]
Add logic for CFLAGS, CXXFLAGS and LDFLAGS to moz.build

I will hold off on any actual conversions before this is reviewed.
Attachment #8377190 - Flags: review?(mshal)
Attachment #8377190 - Flags: review?(mh+mozilla)
Attachment #8377190 - Flags: review?(gps)
Are there any non -l/-L flags for LDFLAGS? if not, please don't include LDFLAGS. I'm going to handle -l/-L flags in bug 921313.
(In reply to Mike Hommey [:glandium] from comment #3)
> Are there any non -l/-L flags for LDFLAGS? if not, please don't include
> LDFLAGS. I'm going to handle -l/-L flags in bug 921313.

It looks like many LDFLAGS are not -l/-L. It has things like -framework, /HEAP:0x40000, -mwindows, etc. A few -l/-L flags get into LDFLAGS from things like TK_LIBS, but I don't see any explicit additions.
Comment on attachment 8377190 [details] [diff] [review]
Add logic for CFLAGS, CXXFLAGS and LDFLAGS to moz.build

Looks good! Though I'd like to echo what jcranmer said in dev-builds - there are probably cases where we want to create a new variable instead of blindly porting everything over (eg: LDFLAGS in xpcom/tests/component_no_aslr might be a candidate).
Attachment #8377190 - Flags: review?(mshal)
Attachment #8377190 - Flags: review?(mh+mozilla)
Attachment #8377190 - Flags: review?(gps)
Attachment #8377190 - Flags: review+
(In reply to Michael Shal [:mshal] from comment #5)
> Comment on attachment 8377190 [details] [diff] [review]
> Add logic for CFLAGS, CXXFLAGS and LDFLAGS to moz.build
> 
> Looks good! Though I'd like to echo what jcranmer said in dev-builds - there
> are probably cases where we want to create a new variable instead of blindly
> porting everything over (eg: LDFLAGS in xpcom/tests/component_no_aslr might
> be a candidate).

Well, I agree in essence, but I have received pushback on things such as bug 973144 which did precisely this.  It's not clear to me where you guys draw the line, and it's not very helpful for me to spend the time writing a patch and then have it not be accepted.

Hence, I may just sit around and not convert much of these variables, except for the cases where I think we obviously don't want to add a new variable.  :/  Not sure how to help more tbh.
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #6)
> (In reply to Michael Shal [:mshal] from comment #5)
> > Comment on attachment 8377190 [details] [diff] [review]
> > Add logic for CFLAGS, CXXFLAGS and LDFLAGS to moz.build
> > 
> > Looks good! Though I'd like to echo what jcranmer said in dev-builds - there
> > are probably cases where we want to create a new variable instead of blindly
> > porting everything over (eg: LDFLAGS in xpcom/tests/component_no_aslr might
> > be a candidate).
> 
> Well, I agree in essence, but I have received pushback on things such as bug
> 973144 which did precisely this.  It's not clear to me where you guys draw
> the line, and it's not very helpful for me to spend the time writing a patch
> and then have it not be accepted.

In that particular case, it sounds like the pushback wasn't so much from adding a new variable, but more from the fact that the build logic is confusing and likely unnecessary in the first place.

I've only looked through LDFLAGS so far, and to me it looks like most of them would be easiest to just keep them in LDFLAGS rather than introducing new variables to do /HEAP:0x40000 or whatever. The one exception I pointed out before was because it used make filtering to remove flags.

> Hence, I may just sit around and not convert much of these variables, except
> for the cases where I think we obviously don't want to add a new variable. 
> :/  Not sure how to help more tbh.

Sorry, I didn't mean to be so discouraging :). I'll try to be less nit-picky so we can move things forward. After all, if something is better off in its own variable, we can still do that after it's moved to moz.build rather than hold you up.
(In reply to comment #8)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #6)
> > (In reply to Michael Shal [:mshal] from comment #5)
> > > Comment on attachment 8377190 [details] [diff] [review]
> > > Add logic for CFLAGS, CXXFLAGS and LDFLAGS to moz.build
> > > 
> > > Looks good! Though I'd like to echo what jcranmer said in dev-builds - there
> > > are probably cases where we want to create a new variable instead of blindly
> > > porting everything over (eg: LDFLAGS in xpcom/tests/component_no_aslr might
> > > be a candidate).
> > 
> > Well, I agree in essence, but I have received pushback on things such as bug
> > 973144 which did precisely this.  It's not clear to me where you guys draw
> > the line, and it's not very helpful for me to spend the time writing a patch
> > and then have it not be accepted.
> 
> In that particular case, it sounds like the pushback wasn't so much from adding
> a new variable, but more from the fact that the build logic is confusing and
> likely unnecessary in the first place.
> 
> I've only looked through LDFLAGS so far, and to me it looks like most of them
> would be easiest to just keep them in LDFLAGS rather than introducing new
> variables to do /HEAP:0x40000 or whatever. The one exception I pointed out
> before was because it used make filtering to remove flags.

Makes sense.  Thanks for taking a look.

> > Hence, I may just sit around and not convert much of these variables, except
> > for the cases where I think we obviously don't want to add a new variable. 
> > :/  Not sure how to help more tbh.
> 
> Sorry, I didn't mean to be so discouraging :). I'll try to be less nit-picky so
> we can move things forward. After all, if something is better off in its own
> variable, we can still do that after it's moved to moz.build rather than hold
> you up.

Well, so the thing is that _my_ goals may not necessarily be aligned with others'.  All I care about for now is to move everything that we need to build libxul into moz.build.  I think in light of that goal, it's OK to clean up some things afterwards, but so far my experience suggests that not everybody is on the same page about that, so depending on who reviews my patch I get different treatment.  :-)

That's not necessarily a problem per se, it's ok for people to disagree, but I'm mostly interested in the above goal, and don't mind doing some cleanup along the way, but may not be that much interested in spending a lot of time on things that don't directly serve that goal for now.  Which is why I am trying to produce patches in small chunks to avoid getting disappointed if I get minused on a huge chunk of changes at once!
I think Ehsan's goal of getting all data required to link libxul into moz.build trumps concerns about variable purity and nomenclature in moz.build.

That doesn't mean we should turn a blind eye to badness at conversion time. I do think it means we can hold off on things like extracting certain linker flags to their own variables/properties.
https://hg.mozilla.org/mozilla-central/rev/588c651748f9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(In reply to comment #10)
> I think Ehsan's goal of getting all data required to link libxul into moz.build
> trumps concerns about variable purity and nomenclature in moz.build.
> 
> That doesn't mean we should turn a blind eye to badness at conversion time. I
> do think it means we can hold off on things like extracting certain linker
> flags to their own variables/properties.

Thanks, does this mean that I should start porting LDFLAGS though?  glandium specifically said to not touch the -l/-L entries there...
Looking back at this, when any of these flags are set in moz.build files, do they show up on the command-lines? I tried both LDFLAGS and CXXFLAGS, and I can see them show up in backend.mk. But because config/config.mk includes backend.mk before these lines:

CFLAGS		= $(OS_CPPFLAGS) $(OS_CFLAGS)
CXXFLAGS	= $(OS_CPPFLAGS) $(OS_CXXFLAGS)
LDFLAGS		= $(OS_LDFLAGS) $(MOZ_FIX_LINK_PATHS)

I think the moz.build definitions get overwritten, correct?
Flags: needinfo?(ehsan)
Hmm, yeah I guess that is a problem.  See bug 975733 for example.

Which one of the solutions proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=975733#c5 I should try?
Flags: needinfo?(ehsan) → needinfo?(mshal)
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #14)
> Hmm, yeah I guess that is a problem.  See bug 975733 for example.
> 
> Which one of the solutions proposed in
> https://bugzilla.mozilla.org/show_bug.cgi?id=975733#c5 I should try?

I don't think moving the 'include backend.mk' around will be easy because it is needed early for FINAL_TARGET (according to bug 900526), so that's out.

I was hoping to just change the CXXFLAGS = $(OS_CPPFLAGS) $(OS_CXXFLAGS) lines, but that seems to be a bit tricky. Both CXXFLAGS and OS_CXXFLAGS initially contain the same value, and are set from configure. So it may look like:

CXXFLAGS = -Wall -Wpointer-arith...
OS_CXXFLAGS = -Wall -Wpointer-arith...

However, some Makefiles append to OS_CXXFLAGS, so if we append to CXXFLAGS in backend.mk, we'll end up with:

CXXFLAGS = -Wall -Wpointer-arith -mozbuildflag
OS_CXXFLAGS = -Wall -Wpointer-arith -makefileflag

which is a little hard to group together into a single set of flags in config.mk (we could use $(sort) to remove duplicates, but that would change the flag ordering too).

Unfortunately I don't quite know the full history of OS_CXXFLAGS vs. CXXFLAGS, so I'm not sure how we ended up in this situation. I would think based on the names that OS_CXXFLAGS should only be set by configure and be unchanged in the front-end build configuration, but that's not what we have.

For now I think it's easiest to just have the backend write out OS_CXXFLAGS where we have CXXFLAGS in moz.build. We can probably do this:

        os_varlist = [
            'CFLAGS',
            'CXXFLAGS',
            'LDFLAGS',
        ]
        for v in os_varlist:
            if v in sandbox and sandbox[v]:
                passthru.variables['OS_'+v] = sandbox[v]

I think we should also not continue this tradition of having both CXXFLAGS and OS_CXXFLAGS in the frontend build files. When we bring over OS_CXXFLAGS, we can just stick them in CXXFLAGS in moz.build (I hope). Once we have all the flags in moz.build, we should be able to fix the emitter & make backend to avoid this extra translation step.

Flagging gps in case he disagrees.
Flags: needinfo?(mshal) → needinfo?(gps)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(And then there's COMPILE_CFLAGS and OS_COMPILE_CFLAGS...)
(In reply to comment #16)
> (And then there's COMPILE_CFLAGS and OS_COMPILE_CFLAGS...)

I think the same kind of solution we come up here should cover those as well... right?
Yes, the interaction between config.mk, backend.mk, rules.mk, and Makefile could be very difficult to deal with. Further complicating matters is the fact that CFLAGS and CXXFLAGS are recursively expanded instead of immediate (they use = instead of :=). Since e.g. |CFLAGS = $(OS_CPPFLAGS) $(OS_CFLAGS)|, modifications to OS_CFLAGS in rules.mk or even Makefile.in *after* the inclusion of rules.mk may get picked up! Thanks, make.

Instead of writing out CFLAGS += or CXXFLAGS += in backend.mk, I'd be very tempted to write out some other variable e.g. MOZBUILD_CFLAGS and have config.mk define CFLAGS in terms of that variable. e.g. CFLAGS = $(OS_CPPFLAGS) $(OS_CFLAGS) $(MOZBUILD_CFLAGS).

I *hope* we can make things work with only one "insertion point" for cflags, somewhere early in config.mk. But, we may need an "early config.mk" and "late config.mk" (and possibly "after rules.mk") stage for variable modifications.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #18)
> I *hope* we can make things work with only one "insertion point" for cflags,
> somewhere early in config.mk. But, we may need an "early config.mk" and
> "late config.mk" (and possibly "after rules.mk") stage for variable
> modifications.

In theory, the only ordering effects of C(XX)FLAGS is the order of include files or counteracting the effect of an earlier option. If we're counteracting an option handled earlier in the same moz.build file, then it doesn't matter where we stick moz.build-defined C{XX}FLAGS in the final compile line. If we're counteracting an option defined in rules.mk, it's better to figure out why and to add some more targeted rules to do that counteraction specifically.

LDFLAGS and linker command lines are a completely different scenario, as the order of arguments is very much a core contract for the command line.
OK, this is too much make language for me, and I'm not really sure what needs to be done here.  Unfortunately that means that I cannot drive this further, which also means that I can't land any of the patches that I have in my queue any more.  It would be nice if someone can help fix the rest of this and unblock me so that I can continue to work on the build system.
Assignee: ehsan → nobody
Blocks: 975731
Blocks: 975733
Blocks: 975734
Blocks: 975735
Blocks: 975736
In case nobody has time to work on this, I would really appreciate if someone can mentor this bug for a community member to draw to the finish line.  Thanks!
Assignee: nobody → Ms2ger
Attachment #8382474 - Flags: review?(gps)
Thanks a lot, Ms2ger!
Attachment #8382474 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/f6d3c2edbc64
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Blocks: 1029974
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.