Closed
Bug 735810
Opened 12 years ago
Closed 12 years ago
Don't try to package MSVC dlls if WIN32_REDIST_DIR is not set, in Firefox
Categories
(Firefox Build System :: General, defect, P3)
Tracking
(firefox11 wontfix, firefox12 wontfix, firefox13 wontfix)
RESOLVED
FIXED
mozilla14
People
(Reporter: sgautherie, Assigned: sgautherie)
References
Details
Attachments
(1 file, 3 obsolete files)
No description provided.
Flags: in-testsuite-
Assignee | ||
Comment 1•12 years ago
|
||
Port SeaMonkey bug 735618. (Untested (yet).) Is there a syntax to add/merge "#ifndef MOZ_WIN32_REDIST" to "#if _MSC_VER != xx00" checks?
Attachment #606120 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 2•12 years ago
|
||
Comment on attachment 606120 [details] [diff] [review] (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set [Backed out: Comment 12] And do you want to remove the "#ifdef XP_WIN32" in package-manifest.in?
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Serge Gautherie (:sgautherie) from comment #1) > (Untested (yet).) Succeeded as https://tbpl.mozilla.org/?tree=Try&rev=1ab2ce9d7277 > Is there a syntax to add/merge "#ifndef MOZ_WIN32_REDIST" to "#if _MSC_VER > != xx00" checks? Eventually found Preprocessor.py and Expression.py: the answer is 'no'. (Though I may investigate a workaround.)
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Serge Gautherie (:sgautherie) from comment #2) > And do you want to remove the "#ifdef XP_WIN32" in package-manifest.in? As this depends on http://mxr.mozilla.org/mozilla-central/source/build/win32/Makefile.in#68 I believe its safer to leave it in.
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Serge Gautherie (:sgautherie) from comment #3) > (Though I may investigate a workaround.) Here it is: I suggest to push the merge of the two patches. Succeeded as https://tbpl.mozilla.org/?tree=Try&rev=9590d17db91d
Attachment #606389 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 6•12 years ago
|
||
Comment on attachment 606120 [details] [diff] [review] (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set [Backed out: Comment 12] It occurred to me that Ted is not listed as a Firefox peer: should probably find someone in that list to review Firefox-only (packaging) patches.
Attachment #606120 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•12 years ago
|
Attachment #606389 -
Flags: review?(gavin.sharp)
Comment 7•12 years ago
|
||
Comment on attachment 606120 [details] [diff] [review] (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set [Backed out: Comment 12] I hereby formally delegate full review authority to Ted!
Attachment #606120 -
Flags: review?(gavin.sharp)
Updated•12 years ago
|
Attachment #606389 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 8•12 years ago
|
||
Comment on attachment 606120 [details] [diff] [review] (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set [Backed out: Comment 12] Whoever reviews these patches first.
Attachment #606120 -
Flags: review?(khuey)
Assignee | ||
Comment 9•12 years ago
|
||
Comment on attachment 606120 [details] [diff] [review] (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set [Backed out: Comment 12] <khuey> sgautherie: I think ted should look at that [bug]
Attachment #606120 -
Flags: review?(khuey)
Comment on attachment 606120 [details] [diff] [review] (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set [Backed out: Comment 12] Review of attachment 606120 [details] [diff] [review]: ----------------------------------------------------------------- I decided to go ahead and do these myself.
Attachment #606120 -
Flags: review?(ted.mielczarek) → review+
Attachment #606389 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 606120 [details] [diff] [review] (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set [Backed out: Comment 12] https://hg.mozilla.org/mozilla-central/rev/b972b89518c3 Cv1 = Av1 + Bv1. [Approval Request Comment] Regression caused by (bug #): (old) User impact if declined: None, but blocks landing bug 713132. Testing completed (on m-c, etc.): Try, this comment. Risk to taking this patch (and alternatives if risky): None, buildtime (+ install) only. String changes made by this patch: None.
Attachment #606120 -
Attachment description: (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set → (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set
[Checked in: See comment 11]
Attachment #606120 -
Flags: approval-mozilla-beta?
Attachment #606120 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #606389 -
Attachment description: (Bv1) removed-files.in: code instead of comment → (Bv1) removed-files.in: code instead of comment
[Checked in: See comment 11]
Assignee | ||
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Turns out our preprocessor doesn't understand #if FOO == bar https://hg.mozilla.org/mozilla-central/rev/9989b866c3a8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #606120 -
Flags: approval-mozilla-beta?
Attachment #606120 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Comment 13•12 years ago
|
||
Historically we have treated the package-manifest.in file as part of the Build Config module, despite it living in the browser/installer directory.
Comment 14•12 years ago
|
||
(In reply to Ted Mielczarek [:ted] from comment #13) > Historically we have treated the package-manifest.in file as part of the > Build Config module, despite it living in the browser/installer directory. FWIW, I consider Firefox's package manifest it to be very much part of the Firefox module, given its purpose. Of course I'm happy to delegate responsibility for reviewing non-controversial changes to build config peers (or anyone else who fully understands the implication of such changes, really).
Comment 15•12 years ago
|
||
Doesn't really matter to me, honestly. :) It's sort of a gray area, but most changes to it are trivial anyway.
Assignee | ||
Updated•12 years ago
|
Attachment #606120 -
Attachment description: (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set
[Checked in: See comment 11] → (Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set
[Backed out: Comment 12]
Attachment #606120 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #606389 -
Attachment description: (Bv1) removed-files.in: code instead of comment
[Checked in: See comment 11] → (Bv1) removed-files.in: code instead of comment
[Backed out: Comment 12]
Attachment #606389 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 years ago
|
||
Cv1, but moving new logic to Makefile, as Preprocessor.py supports only 1 level of substitution :-| http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/sgautherie.bz@free.fr-58b0579a1cac/try-win64/firefox-14.0a1.en-US.win64-x86_64.zip removed-files is correct now.
Attachment #608985 -
Flags: review?(khuey)
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to Serge Gautherie (:sgautherie) from comment #16) > as Preprocessor.py supports only 1 level of substitution :-| Ftr, https://developer.mozilla.org/en/Build/Text_Preprocessor http://mxr.mozilla.org/mozilla-central/source/config/Preprocessor.py http://mxr.mozilla.org/mozilla-central/source/config/Expression.py
Comment 18•12 years ago
|
||
Try run for 58b0579a1cac is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=58b0579a1cac Results (out of 14 total builds): success: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/sgautherie.bz@free.fr-58b0579a1cac
Assignee | ||
Updated•12 years ago
|
Attachment #608985 -
Flags: review?(ted.mielczarek)
Comment 19•12 years ago
|
||
Comment on attachment 608985 [details] [diff] [review] (Cv2) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set Review of attachment 608985 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/installer/Makefile.in @@ +95,5 @@ > DEFINES += -DMOZ_CHILD_PROCESS_NAME=$(MOZ_CHILD_PROCESS_NAME) > > +# Set MSVC dlls version to package, if any. > +ifndef WIN32_REDIST_DIR > +DEFINES += -DMOZ_MSVC_REDIST=-1 This doesn't seem necessary. If MOZ_MSVC_REDIST is not defined, then all your #if conditions will evaluate to false.
Attachment #608985 -
Flags: review?(ted.mielczarek)
Attachment #608985 -
Flags: review?(khuey)
Attachment #608985 -
Flags: review-
Assignee | ||
Comment 20•12 years ago
|
||
Cv2, with comment 19 suggestion(s).
Attachment #608985 -
Attachment is obsolete: true
Attachment #610544 -
Flags: review?(ted.mielczarek)
Updated•12 years ago
|
Attachment #610544 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 21•12 years ago
|
||
Comment on attachment 610544 [details] [diff] [review] (Cv3) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set [Checked in: Comment 21] http://hg.mozilla.org/mozilla-central/rev/92fe907ddac8
Attachment #610544 -
Attachment description: (Cv3) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set → (Cv3) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set
[Checked in: Comment 21]
Assignee | ||
Updated•12 years ago
|
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: Build Config → General
Product: Firefox → Firefox Build System
Updated•5 years ago
|
Target Milestone: Firefox 14 → mozilla14
You need to log in
before you can comment on or make changes to this bug.
Description
•