751695 - Make our builders pick the correct mozconfig on Windows based on whether they're on a 32-bit or a 64-bit platform

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(no longer active)]

I'm looking for a way to make our default Win32 mozconfigs do the right thing based on whether the builder is a Win32 or a Win64 builder.  This should make it possible for RelEng to gradually move our Windows builds to the Win64 builders, so that we can take advantage of the additional 1G of address space.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 620854 [details] [diff] [review]
Patch (v1)

This seems like it'd be more simply implemented as:

if test "$PROCESSOR_ARCHITECTURE" = "AMD64" -o "$PROCESSOR_ARCHITEW6432" = "AMD64"; then

Comment 3

[JP Rosevear [:jpr]]

(In reply to Ehsan Akhgari [:ehsan] from comment #0)
> I'm looking for a way to make our default Win32 mozconfigs do the right
> thing based on whether the builder is a Win32 or a Win64 builder.  This
> should make it possible for RelEng to gradually move our Windows builds to
> the Win64 builders, so that we can take advantage of the additional 1G of
> address space.

Are we sure we want to do this incrementally?  Generating builds on two different OSes seems a little scary.

Comment 4

[(no longer active)]

(In reply to Ted Mielczarek [:ted] from comment #2)
> Comment on attachment 620854 [details] [diff] [review]
> Patch (v1)
> 
> This seems like it'd be more simply implemented as:
> 
> if test "$PROCESSOR_ARCHITECTURE" = "AMD64" -o "$PROCESSOR_ARCHITEW6432" =
> "AMD64"; then

I tested this.  Those variables don't seem to be reliably set for some reason.  Using IsWow64Process is a much more reliable way to gather this information.

Comment 5

[(no longer active)]

(In reply to JP Rosevear [:jpr] from comment #3)
> (In reply to Ehsan Akhgari [:ehsan] from comment #0)
> > I'm looking for a way to make our default Win32 mozconfigs do the right
> > thing based on whether the builder is a Win32 or a Win64 builder.  This
> > should make it possible for RelEng to gradually move our Windows builds to
> > the Win64 builders, so that we can take advantage of the additional 1G of
> > address space.
> 
> Are we sure we want to do this incrementally?  Generating builds on two
> different OSes seems a little scary.

I'm not convinced that it is, really.  We're using the same compiler, the only thing which is different is the OS CPU architecture, which should not make any difference in the code that the compiler generates.  Also, note that we'll at least switch all PGO builds to Win64 builders, which means that all of our release builds are always generated on Win64 machines, so technically we'll be using the same OS to build all release binaries.

Comment 6

[(no longer active)]

(By "release" I mean Nightly, Aurora, Beta and Firefox.)

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> (In reply to JP Rosevear [:jpr] from comment #3)
> > Are we sure we want to do this incrementally?  Generating builds on two
> > different OSes seems a little scary.
> 
> I'm not convinced that it is, really.  We're using the same compiler, the
> only thing which is different is the OS CPU architecture, which should not
> make any difference in the code that the compiler generates.

Famous last words. ;)  A number of things can go wrong with compiler reproducibility; I do think it'd be prudent to compare Win32-built binaries with Win64-built binaries before checking this in.

> Also, note
> that we'll at least switch all PGO builds to Win64 builders, which means
> that all of our release builds are always generated on Win64 machines, so
> technically we'll be using the same OS to build all release binaries.

This does lower the risk factor somewhat.  But it still leaves open weird build machine-dependent bugs (on try, for instance) during the switchover.

Comment 8

[(no longer active)]

(In reply to Nathan Froyd (:froydnj) from comment #7)
> (In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > (In reply to JP Rosevear [:jpr] from comment #3)
> > > Are we sure we want to do this incrementally?  Generating builds on two
> > > different OSes seems a little scary.
> > 
> > I'm not convinced that it is, really.  We're using the same compiler, the
> > only thing which is different is the OS CPU architecture, which should not
> > make any difference in the code that the compiler generates.
> 
> Famous last words. ;)  A number of things can go wrong with compiler
> reproducibility; I do think it'd be prudent to compare Win32-built binaries
> with Win64-built binaries before checking this in.

For sure.  That's what bug 751694 is about.  We can also test this on the try server once this patch is in.

But do you have any specific concerns?

> > Also, note
> > that we'll at least switch all PGO builds to Win64 builders, which means
> > that all of our release builds are always generated on Win64 machines, so
> > technically we'll be using the same OS to build all release binaries.
> 
> This does lower the risk factor somewhat.  But it still leaves open weird
> build machine-dependent bugs (on try, for instance) during the switchover.

I'm not sure what you mean.  We already have the problem of PGO specific bugs which do not show up in our non-PGO testing builds which we do much more regularly on central/inbound.  So in a sense the risk of the difference between PGO builds on Win64 builders and non-PGO builds on Win32 builders is something that we're already dealing with, roughly speaking.

Comment 9

[(not currently active) Ted Mielczarek]

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> I tested this.  Those variables don't seem to be reliably set for some
> reason.  Using IsWow64Process is a much more reliable way to gather this
> information.

Really? They work fine from both cmd and msys on my Windows 7 x64. These are actually recommended by MSDN!

Comment 10

[(no longer active)]

(In reply to Ted Mielczarek [:ted] from comment #9)
> (In reply to Ehsan Akhgari [:ehsan] from comment #4)
> > I tested this.  Those variables don't seem to be reliably set for some
> > reason.  Using IsWow64Process is a much more reliable way to gather this
> > information.
> 
> Really? They work fine from both cmd and msys on my Windows 7 x64. These are
> actually recommended by MSDN!

When I tested this yesterday on the builder, it did not work.  I gave up access to the builder, but I can file a new bug to get access to another builder and retest if you think this is worth it.

Comment 11

[Mozilla RelEng Bot]

Try run for 3b50adb710a5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3b50adb710a5
Results (out of 2 total builds):
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3b50adb710a5

Comment 12

[Mozilla RelEng Bot]

Try run for 3db3cd41f7c8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3db3cd41f7c8
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3db3cd41f7c8

Comment 13

[(no longer active)]

Attachment: Patch (v2)

Using environment variables.

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 621123 [details] [diff] [review]
Patch (v2)

Review of attachment 621123 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/config/mozconfigs/win32/debug
@@ +10,5 @@
> +if test "$PROCESSOR_ARCHITECTURE" = "AMD64" -o "$PROCESSOR_ARCHITEW6432" = "AMD64"; then
> +  . $topsrcdir/build/win32/mozconfig.vs2010-win64
> +else
> +  . $topsrcdir/build/win32/mozconfig.vs2010
> +fi

Kind of wondering if this doesn't deserve to be in its own separate file that then sources the proper file. Am I overcomplicating things?

Comment 15

[(no longer active)]

(In reply to Ted Mielczarek [:ted] from comment #14)
> Comment on attachment 621123 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 621123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/config/mozconfigs/win32/debug
> @@ +10,5 @@
> > +if test "$PROCESSOR_ARCHITECTURE" = "AMD64" -o "$PROCESSOR_ARCHITEW6432" = "AMD64"; then
> > +  . $topsrcdir/build/win32/mozconfig.vs2010-win64
> > +else
> > +  . $topsrcdir/build/win32/mozconfig.vs2010
> > +fi
> 
> Kind of wondering if this doesn't deserve to be in its own separate file
> that then sources the proper file. Am I overcomplicating things?

Yes!  :P

Comment 16

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d944ef78da84

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d944ef78da84