Last Comment Bug 735810 - Don't try to package MSVC dlls if WIN32_REDIST_DIR is not set, in Firefox
: Don't try to package MSVC dlls if WIN32_REDIST_DIR is not set, in Firefox
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Build Config (show other bugs)
: Trunk
: All Windows XP
: P3 normal (vote)
: Firefox 14
Assigned To: Serge Gautherie (:sgautherie)
:
: Gregory Szorc [:gps]
Mentors:
Depends on: 737435
Blocks: 713132 735618
  Show dependency treegraph
 
Reported: 2012-03-14 12:18 PDT by Serge Gautherie (:sgautherie)
Modified: 2013-06-10 05:29 PDT (History)
4 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
wontfix


Attachments
(Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set [Backed out: Comment 12] (2.91 KB, patch)
2012-03-14 22:46 PDT, Serge Gautherie (:sgautherie)
khuey: review+
Details | Diff | Splinter Review
(Bv1) removed-files.in: code instead of comment [Backed out: Comment 12] (1.42 KB, patch)
2012-03-15 16:04 PDT, Serge Gautherie (:sgautherie)
khuey: review+
Details | Diff | Splinter Review
(Cv2) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set (3.21 KB, patch)
2012-03-24 04:39 PDT, Serge Gautherie (:sgautherie)
ted: review-
Details | Diff | Splinter Review
(Cv3) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set [Checked in: Comment 21] (3.17 KB, patch)
2012-03-29 07:48 PDT, Serge Gautherie (:sgautherie)
ted: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-03-14 12:18:22 PDT

    
Comment 1 Serge Gautherie (:sgautherie) 2012-03-14 22:46:12 PDT
Created attachment 606120 [details] [diff] [review]
(Av1) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set
[Backed out: Comment 12]

Port SeaMonkey bug 735618.
(Untested (yet).)

Is there a syntax to add/merge "#ifndef MOZ_WIN32_REDIST" to "#if _MSC_VER != xx00" checks?
Comment 2 Serge Gautherie (:sgautherie) 2012-03-14 22:49:52 PDT
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?
Comment 3 Serge Gautherie (:sgautherie) 2012-03-15 12:17:21 PDT
(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.)
Comment 4 Serge Gautherie (:sgautherie) 2012-03-15 12:40:42 PDT
(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.
Comment 5 Serge Gautherie (:sgautherie) 2012-03-15 16:04:54 PDT
Created attachment 606389 [details] [diff] [review]
(Bv1) removed-files.in: code instead of comment
[Backed out: Comment 12]

(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
Comment 6 Serge Gautherie (:sgautherie) 2012-03-16 10:26:29 PDT
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.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-16 10:54:25 PDT
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!
Comment 8 Serge Gautherie (:sgautherie) 2012-03-17 23:07:36 PDT
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.
Comment 9 Serge Gautherie (:sgautherie) 2012-03-18 15:36:04 PDT
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]
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-19 17:45:21 PDT
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.
Comment 11 Serge Gautherie (:sgautherie) 2012-03-19 19:11:48 PDT
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.
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-20 09:10:02 PDT
Turns out our preprocessor doesn't understand #if FOO == bar

https://hg.mozilla.org/mozilla-central/rev/9989b866c3a8
Comment 13 Ted Mielczarek [:ted.mielczarek] 2012-03-20 11:20:05 PDT
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 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-20 15:36:58 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2012-03-21 08:15:12 PDT
Doesn't really matter to me, honestly. :) It's sort of a gray area, but most changes to it are trivial anyway.
Comment 16 Serge Gautherie (:sgautherie) 2012-03-24 04:39:54 PDT
Created attachment 608985 [details] [diff] [review]
(Cv2) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set

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.
Comment 17 Serge Gautherie (:sgautherie) 2012-03-24 04:52:35 PDT
(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 Mozilla RelEng Bot 2012-03-24 05:04:02 PDT
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
Comment 19 Ted Mielczarek [:ted.mielczarek] 2012-03-27 09:30:48 PDT
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.
Comment 20 Serge Gautherie (:sgautherie) 2012-03-29 07:48:31 PDT
Created attachment 610544 [details] [diff] [review]
(Cv3) Stop trying to package MSVC dlls when WIN32_REDIST_DIR isn't set
[Checked in: Comment 21]

Cv2, with comment 19 suggestion(s).
Comment 21 Serge Gautherie (:sgautherie) 2012-03-29 11:17:20 PDT
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

Note You need to log in before you can comment on or make changes to this bug.