Closed Bug 444022 Opened 12 years ago Closed 5 years ago

Duplicated build options in about:buildconfig

Categories

(MailNews Core :: Build Config, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED WORKSFORME
Thunderbird 3.1b2

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

Attachments

(2 files, 1 obsolete file)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a1pre) Gecko/2008070706 SeaMonkey/2.0a1pre] (calemaisu-test-win32/1215437056) (W2Ksp4)

<about:buildconfig> nits, fwiw:

(1.9.0) usual nightly has
{{
--enable-application=suite --enable-update-channel=nightly --enable-update-packaging --disable-debug --enable-optimize --enable-jemalloc
}}

(1.9.1) calemaisu tinderbox build has
{{
--enable-application=suite --enable-update-channel=nightly --enable-update-packaging --disable-debug --enable-optimize --enable-jemalloc

--disable-tests --enable-application=../suite --disable-official-branding --disable-calendar --disable-debug --enable-optimize --cache-file=.././config.cache --srcdir=/e/builds/slave/calemaisu-test-win32/build/mozilla
}}
as if getting both/duplicated "1.9.0 options" and "1.9.1 options"...

KaiRo commented:
{{
Not sure what we can do about that, this is because the Mozilla configure is called as a sub-configure of our own configure and therfore handed additional options.
I'm sure we can live with that for now.
}}

Filing it here: only good to be aware of it now, then look into it later.
Summary: See if we can cleanup duplicated build options in new build config → Duplicated build options in about:buildconfig
QA Contact: build-config → build-config
Keywords: helpwanted, polish
OS: Windows 2000 → All
Hardware: x86 → All
1st part, to start with.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #428112 - Flags: review?(bugspam.Callek)
Flags: in-testsuite-
Keywords: helpwanted, polish
Product: SeaMonkey → MailNews Core
QA Contact: build-config → build-config
Target Milestone: --- → Thunderbird 3.1b1
Did you make absolutely sure that all the options you are removing are still recognized and CORRECTLY used by ALL the subconfigures that we call?

I'd like to have both Callek and Standard8 review this patch, if possible, removing some of those lines sounds too risky for me to have a single person look at them.
(In reply to comment #2)

> Did you make absolutely sure that all the options you are removing are still
> recognized and CORRECTLY used by ALL the subconfigures that we call?

ALL? That's 'mozilla' and 'directory/c-sdk' only!
CORRECTLY? Duplication shouldn't be needed (or there _would_ be a problem) and removing the optional values seemed like an inconsistency (or we should document/fix that).
I did a basic test with 1 option with 'mozilla' and visually checked the rest in (1+2) 'configure.in'.

> I'd like to have both Callek and Standard8 review this patch, if possible,
> removing some of those lines sounds too risky for me to have a single person
> look at them.

I'll let Callek (or you) make the actual call, given Standard8's review delay.
I'm pretty sure a relative path in l10n-base will give trouble, for one thing.

I don't care if he has review delays, this patch only has cosmetic benefit, but could break things, so I want to be sure.
Attachment #428112 - Flags: review?(bugzilla)
(In reply to comment #4)

> I'm pretty sure a relative path in l10n-base will give trouble, for one thing.

I know little about l10n but I assume you're right:
yet this means the current code has this flaw already.

If someone confirms this does need fixing, I'll leave this block as is (and add a fix reminder) for now.

> I don't care if he has review delays, this patch only has cosmetic benefit

(It also takes Mark's time away from other patches/bugs, but as you wish.)
(In reply to comment #5)
> (In reply to comment #4)
> 
> > I'm pretty sure a relative path in l10n-base will give trouble, for one thing.
> 
> I know little about l10n but I assume you're right:
> yet this means the current code has this flaw already.

No, as the code on our side fixes the variable up to be absolute and we hand that absolute path to the mozilla build system, which fixes up the problem (esp. as the last argument counts if we hand over multiple identical ones). This is similar to the trick we're doing with --enable-application.
Av1, with comment 6 suggestion(s).

(In reply to comment #6)
> the code on our side fixes the variable up to be absolute

You're absolutely (;-)) right: I read that line and didn't recognize the pattern :-/
Attachment #428112 - Attachment is obsolete: true
Attachment #428207 - Flags: review?(bugzilla)
Attachment #428207 - Flags: review?(bugspam.Callek)
Attachment #428112 - Flags: review?(bugzilla)
Attachment #428112 - Flags: review?(bugspam.Callek)
Comment on attachment 428207 [details] [diff] [review]
(Av2) Remove some duplicated/inexistent options, Do some rewrite and documentation
[Checkin: See comment 12]

looks good, from my understanding. I'm holding off my official review until I get a chance to test.
Comment on attachment 428207 [details] [diff] [review]
(Av2) Remove some duplicated/inexistent options, Do some rewrite and documentation
[Checkin: See comment 12]

doesn't [seem to] break anything on my end. Wouldn't hurt to run this through try either though.
Attachment #428207 - Flags: review?(bugspam.Callek) → review+
(In reply to comment #9)
> Wouldn't hurt to run this through try either though.

Passed as
http://build.mozillamessaging.com/buildbot/try/changes/387
Comment on attachment 428207 [details] [diff] [review]
(Av2) Remove some duplicated/inexistent options, Do some rewrite and documentation
[Checkin: See comment 12]

I've done a bit of testing and investigation and removing addition of these options seems fine.
Attachment #428207 - Flags: review?(bugzilla) → review+
Comment on attachment 428207 [details] [diff] [review]
(Av2) Remove some duplicated/inexistent options, Do some rewrite and documentation
[Checkin: See comment 12]


http://hg.mozilla.org/comm-central/rev/b0fc16028656
Av2, with s/#/dnl/g, test _-n_ and restoring ending 'fi # ...'.
Attachment #428207 - Attachment description: (Av2) Remove some duplicated/inexistent options, Do some rewrite and documentation → (Av2) Remove some duplicated/inexistent options, Do some rewrite and documentation [Checkin: See comment 12]
Target Milestone: Thunderbird 3.1b1 → Thunderbird 3.1b2
Comment on attachment 435348 [details] [diff] [review]
(Bv1) Remove initial options, Do some rewrite and documentation.

punting this review; I don't want to have my name attached to changing this magic, at this point.
Attachment #435348 - Flags: review?(bugspam.Callek) → review?(kairo)
Comment on attachment 435348 [details] [diff] [review]
(Bv1) Remove initial options, Do some rewrite and documentation.

I also don't feel compelled to even look into this right now, I feel too swamped with other work and too little motivated to make sure twisting this magic doesn't break anything while the benefits of it are doubtful anyhow.
Perhaps you have more luck with Standard8.
Attachment #435348 - Flags: review?(kairo) → review?(bugzilla)
Comment on attachment 435348 [details] [diff] [review]
(Bv1) Remove initial options, Do some rewrite and documentation.

If this is to simplify what we pass m-c, then I think the cost far outweighs the benefit.

Additionally, I think if this is to simplify the about:buildconfig options, then again the cost outweighs the benefit with this solution. Other solutions are potentially possible but I can't think of a viable one at the moment.
Attachment #435348 - Flags: review?(bugzilla) → review-
Somewhere in build system history, this was fixed.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.