1128186 - Rearchitect the various MozillaBuild batch files

Status: RESOLVED FIXED

(Firefox Build System :: MozillaBuild, task)

Description

[Ryan VanderMeulen [:RyanVM]]

I'm filing this bug to track completing the intended batch file reworking for MozillaBuild 2.0.

My plan is:
1) Have one primary batch file to launch a shell and set appropriate environment variables.
  * This means renaming start-l10n.bat to something more suitable and merging the guess-msvc and parts of the start-msvc functionality into it.
2) Leave the start-msvc batch files as thin wrappers around start-shell.bat that invoke it with different command line parameters to start the appropriate environment.
  * This should be much more easily doable with the cleanups from the other bugs.
  * Not necessary an immediate-term goal for this refactoring, but this could also potentially enable someone to switch compiler environments on the fly down the road.

This should make maintenance much simpler down the road as all the primary logic is consolidated to one place. It also reduces complexity by removing batch files with unclear purposes.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: Rename start-shell-l10n.bat to start-shell.bat

Whoopity doo.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: Kill guess-msvc.bat and move most of the start-shell-msvc functionality into start-shell.bat

This is the meat of the work. The start-shell-msvc* batch files have been reduced to just setting a few variables and calling start-shell.bat. start-shell.bat now handles setting up the MSYS environment and the MSVC environment if invoked as such.

I made start-shell.bat as generic as possible so hopefully adding support for new MSVC versions requires nothing more than copying and modifying the trivial MSVC batch scripts. I've verified locally with MSVC2013 that things seem to work as expected.

To make reviewing this easier, I'm also going to attach a copy of the resulting start-shell.bat by itself as well.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Attachment: Remove profile-echo.sh

Not sure what purpose it was ever intended to fulfill, but it seems entirely redundant to me. Removing it to generate a little less console spam on startup.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Attachment: start-shell.bat

This is what start-shell.bat looks like after Part 2.

Joel: How does this look to you? Any style/functional concerns?

Brian: I don't have MSVC2015 handy to test. Can you try these modified scripts out as well and confirm that you get the expected results with both the 32bit and 64bit versions? To make it easier to try, here's a zip of all 5 changed files:
http://people.mozilla.org/~rvandermeulen/startscripts.zip

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bail earlier if the x64 MSVC start script is invoked on a 32-bit host OS

Previously, this scenario would have hit the "Unable to call a suitable vcvars script. Exiting." error anyway. This instead adds an earlier check for it and quits with a more-useful error message.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

I created a Win7 32-bit VM and was able to confirm that all 4 start-msvc* scripts give the expected results for both MSVC versions.

Comment 7

[Joel Rocha]

I've seen your changes: Assuming that the script behavior is working as intended, i've checked your code and i'll propose some minor changes.

This file looks much more cleaner. I wonder what the older versions were all about :P

I've also seen you "Fail Fast" change on comment #5. And i subscribe it.

Nice work Ryan! ;)

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #4)
> Created attachment 8557562 [details]
> start-shell.bat
> 
> This is what start-shell.bat looks like after Part 2.
> 
> Joel: How does this look to you? Any style/functional concerns?
> 
> Brian: I don't have MSVC2015 handy to test. Can you try these modified
> scripts out as well and confirm that you get the expected results with both
> the 32bit and 64bit versions? To make it easier to try, here's a zip of all
> 5 changed files:
> http://people.mozilla.org/~rvandermeulen/startscripts.zip

Comment 8

[Joel Rocha]

Attachment: start-shell.bat.diff

Minor changes to your start-shell.bat
I'm assuming the variable %MOZBUILDDIR% is only used in this script, and no other further script depends on it, so i've changed the file so that such variable is not set.

Other improvements were added such as "Fail Fast" code blocks to fail as soon as possible without executing any other calls.

Comment 9

[Joel Rocha]

Comment on attachment 8557562 [details]
start-shell.bat

Looks good, much more cleaner, and easier to understand. I have some functional changes, but i'll propose them on a patch.

Comment 10

[Joel Rocha]

Attachment: start-shell.bat.diff

Same as the previous file but with the Ryan VM "Fail Fast" code block proposed on comment #5

Comment 11

[Joel Rocha]

Attachment: start-shell.bat

This is not a new file but a patch to an already existent file.

Comment 12

[Joel Rocha]

Attachment: start-shell.bat

Where it reads:
SET SDKDIR=%%B

Shall be read:
SET SDKDIR="%%B"

As the %%B variable might contain directories with spaces: It's safer to do it this way.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8557562 [details]
start-shell.bat

I've already tested this myself with 2015 and verified that it works as expected.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8561039 [details] [diff] [review]
start-shell.bat

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

Overall, I like this patch. I'll commit a cleaned-up version of this (with review comments addressed and whitespace fix-ups) once the other ones get review. Thanks, Joel!

::: start-shell.bat.ryan
@@ +31,5 @@
> +	    PAUSE
> +	    EXIT /B 1
> +  )
> +  
> +  SET PATH=%MOZBUILDDIR%moztools\bin;%PATH%

I like the idea of putting the early-quit in this block and fold it into attachment 8558706 [details] [diff] [review] accordingly. However, I actually intentionally chose to leave the moztools path setting in a separate block because I wanted leave things more logically split out into individual steps. 

Even if the resulting code is a bit more verbose than it has to be, I think it makes it easier to follow and understand. That said, I'm also still hoping to completely kill the moztools package, which would negate that bit anyway :)

@@ +55,5 @@
>        PAUSE
>        EXIT /B 1
>      )
> +	
> +	FOR /F "tokens=2*" %%A IN ('REG QUERY !MSVCKEY! /v ProductDir') DO SET VCDIR=%%B

Agreed that this makes things a bit more readable (testing for the error condition directly and bailing).

@@ +87,5 @@
> +        ECHO.
> +        PAUSE
> +        EXIT /B 1
> +      )
> +	  

I like this a lot better. I wasn't a huge fan of setting SDKDIR and then un-setting it later.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(In reply to Joel Rocha from comment #8)
> I'm assuming the variable %MOZBUILDDIR% is only used in this script, and no
> other further script depends on it, so i've changed the file so that such
> variable is not set.

Looks like that change got lost? That said, it's a valid point. I'll include that change in the cleaned-up patch.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Some day, we'll probably want to spruce this all up with some GOTO love too so we can avoid some of the nested logic. But we'll fight that fight another day when needed (probably around the time we need to support more than one Windows SDK again).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bail earlier if the x64 MSVC start script is invoked on a 32-bit host OS

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Attachment: Other cleanups

This is Joel's patch, rebased to apply cleanly on top of the other patches and with some whitespace fixes.

Also, I dropped the quote change on the SDKDIR to be consistent with our VCDIR usage. For aesthetic reasons, I'd rather just make sure we use quotes properly if/when we use SDKDIR for more than just an echo like we are currently.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Attachment: start-shell.bat

The current state of start-shell.bat with all 5 patches applied.

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 8557561 [details] [diff] [review]
Remove profile-echo.sh

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

I've found this useful in the past as a sanity check on what compiler version I launched a shell for. I suspect that was its only real purpose in life. I won't veto you removing it, but I think it does have value.

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 8557560 [details] [diff] [review]
Kill guess-msvc.bat and move most of the start-shell-msvc functionality into start-shell.bat

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

Pretty nice cleanup and consolidation!

::: start-shell.bat
@@ +93,5 @@
> +  ECHO Windows SDK Directory: !SDKDIR!
> +
> +  REM Prepend MSVC paths.
> +  IF "%MOZ_MSVCBITS%" == "32" (
> +    REM Prefer cross-compiling 32-bit builds using the 64-bit toolchain if able to do so.

Doesn't this need to be inside an IF "%WIN64%" == "1" ( ? (Otherwise you'll be trying to use the cross-toolchain on 32-bit Windows.)

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 8557561 [details] [diff] [review]
Remove profile-echo.sh

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

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #20)
> I've found this useful in the past as a sanity check on what compiler
> version I launched a shell for. I suspect that was its only real purpose in
> life. I won't veto you removing it, but I think it does have value.

After inspecting the output of a Mozillabuild shell prompt launch, I see where you're coming from. We already output plenty of descriptive text there.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8557561 [details] [diff] [review]
Remove profile-echo.sh

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #21)
> Doesn't this need to be inside an IF "%WIN64%" == "1" ( ? (Otherwise you'll
> be trying to use the cross-toolchain on 32-bit Windows.)

No, because the amd64_x86 toolchain doesn't get installed on 32-bit host OSes (confirmed looking at the 32-bit Win7 VM I set up for testing these changes). Though you're right that it would be a good sanity check to be on the safe side, anyway.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22)
> After inspecting the output of a Mozillabuild shell prompt launch, I see
> where you're coming from. We already output plenty of descriptive text there.

Actually, the more I think about it, I think I actually want to go the route of keeping profile-echo.sh around and having it do all the text output under normal non-error conditions. This is particularly important if/when we switch to minTTY since it launches in a new window and we'd otherwise see nothing when all's said and done.

New patch coming :)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Attachment: Kill guess-msvc.bat and move most of the start-shell-msvc functionality into start-shell.bat

This adds the sanity check for making sure that we're on a 64-bit host OS before trying to use the amd64_x86 cross-compile toolchain.

Comment 25

[Joel Rocha]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #19)
> Created attachment 8561123 [details]
> start-shell.bat
> 
> The current state of start-shell.bat with all 5 patches applied.

Warning: This bit is still missing:

"Where it reads:
SET SDKDIR=%%B

Shall be read:
SET SDKDIR="%%B"

As the %%B variable might contain directories with spaces: It's safer to do it this way."

The current state of the script might work, but i think there can be cases were it doesnt.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

(In reply to Joel Rocha from comment #25)
> Warning: This bit is still missing:

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18)
> Also, I dropped the quote change on the SDKDIR to be consistent with our
> VCDIR usage. For aesthetic reasons, I'd rather just make sure we use quotes
> properly if/when we use SDKDIR for more than just an echo like we are
> currently.

Comment 27

[Joel Rocha]

Ok, thank you Ryan for the 'heads up!' :)

Well if you want to be consistent, they all should be changed, unless what comes from 'REG QUERY !MSVCKEY! /v ProductDir' is always already enclosed in quotes. (i forgot about that. If you don't know either, you can test it and check if that's the case)

Thank you for your reply anyway ;)

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #26)
> (In reply to Joel Rocha from comment #25)
> > Warning: This bit is still missing:
> 
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18)
> > Also, I dropped the quote change on the SDKDIR to be consistent with our
> > VCDIR usage. For aesthetic reasons, I'd rather just make sure we use quotes
> > properly if/when we use SDKDIR for more than just an echo like we are
> > currently.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Yes, I already verified that.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Attachment: Move all non-error output to profile-echo.sh

This patch does a few things:
1) Moves all of the non-error console output to profile-echo.sh. As expected, this works with minTTY as well.
2) Refactors the early-quit logic into a single _QUIT function, cutting down on a lot of duplicated code. Now the error handling throughout the script amounts to setting a specific error string and going to the early quit code.

I'm probably going to write one more patch for this bug still, because I'm still not quite happy with the MSVC path prepending logic yet. The problem with the logic now is that if for some reason vcvarsamd64_x86.bat isn't found, we won't even try to call vcvars32.bat as a last resort. Not that such a scenario should be possible since the Visual Studio installer doesn't allow for that option, but neither should vcvarsamd64_x86.bat being present on a 32-bit system :)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Attachment: Fall back to vcvars32.bat if vcvarsamd64_x86.bat isn't run

The problem with guarding vcvarsamd64_x86.bat on WIN64 is that it effectively broke falling back on to vcvars32.bat. This restores that functionality.

This is the final patch I intend to post to this bug, so reviews welcome :)

Comment 31

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-build/rev/f7984b01fd12
http://hg.mozilla.org/mozilla-build/rev/27857f1af9bf
http://hg.mozilla.org/mozilla-build/rev/5a5c65720e70
http://hg.mozilla.org/mozilla-build/rev/ddac755951ce
http://hg.mozilla.org/mozilla-build/rev/879c7f92bc48
http://hg.mozilla.org/mozilla-build/rev/c9cdbb18278a