Closed
Bug 444022
Opened 16 years ago
Closed 10 years ago
Duplicated build options in about:buildconfig
Categories
(MailNews Core :: Build Config, defect)
MailNews Core
Build Config
Tracking
(Not tracked)
RESOLVED
WORKSFORME
Thunderbird 3.1b2
People
(Reporter: sgautherie, Assigned: sgautherie)
References
()
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Callek
:
review+
standard8
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
standard8
:
review-
|
Details | Diff | Splinter Review |
[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.
Updated•16 years ago
|
Summary: See if we can cleanup duplicated build options in new build config → Duplicated build options in about:buildconfig
Updated•15 years ago
|
QA Contact: build-config → build-config
Updated•15 years ago
|
Assignee | ||
Comment 1•15 years ago
|
||
1st part, to start with.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #428112 -
Flags: review?(bugspam.Callek)
Assignee | ||
Updated•15 years ago
|
Blocks: CcConfCleanup
Flags: in-testsuite-
Keywords: helpwanted,
polish
Product: SeaMonkey → MailNews Core
QA Contact: build-config → build-config
Target Milestone: --- → Thunderbird 3.1b1
Comment 2•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
(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.
Comment 4•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #428112 -
Flags: review?(bugzilla)
Assignee | ||
Comment 5•15 years ago
|
||
(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.)
Comment 6•15 years ago
|
||
(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.
Assignee | ||
Comment 7•15 years ago
|
||
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 8•15 years ago
|
||
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 9•15 years ago
|
||
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+
Assignee | ||
Comment 10•15 years ago
|
||
(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 11•15 years ago
|
||
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+
Assignee | ||
Comment 12•15 years ago
|
||
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]
Assignee | ||
Updated•15 years ago
|
Target Milestone: Thunderbird 3.1b1 → Thunderbird 3.1b2
Comment 14•15 years ago
|
||
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 15•15 years ago
|
||
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 16•15 years ago
|
||
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-
Comment 17•10 years ago
|
||
Somewhere in build system history, this was fixed.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•