Mach: Improve Visual Studio detection for non-standard-path installations

[MeFisto94]

Improve locating the Visual Studio installation

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #25225
• These changes do not require tests because testing build infrastructure is difficult

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @paulrouget (or someone else) soon.

[highfive]

Heads up! This PR modifies the following files:

• @wafflespeanut: python/servo/command_base.py

[MeFisto94]

Explanation:
As you can see, the old code only relied on trying a fixed path to locate visual studio, but this fails for me.
This code essentially comes from https://searchfox.org/mozilla-central/rev/923eec8d2fb8078ebc7a33a9e1ce73eac01f7446/build/moz.configure/toolchain.configure#581 but has been simplified and cut down, so that it fits into servo.

I understand if you don't want to maintain/integrate these changes, but it might help others being stuck in the same situation. I've also did it so that the default behavior is kept and only replaced when the operation fails.
In theory, it would be better to only use the _ext version, because the current version depends on version names (Community, Professional) as well as requireing manual edits to it, everytime there is a new VS Version.
My code only requires maintaining "Current" for MSBuild Version and unfortunately Microsoft.VisualStudio.Component.Windows10SDK, which according to https://docs.microsoft.com/de-de/visualstudio/install/workload-component-id-vs-professional?view=vs-2019 is the name of Windows Universal C Runtime, which is required by servo according to the docs.

There are problems (apart from the "political" decision about maintenance and whethere this code should be touched or not):

• I forgot that get_msbuild_version uses an even stranger version format instead of following the VS Version ("Current", but then also supporting "14.0", which isn't supported by find_highest_msvc_version). I will force-push this asap, but to me that feels odd.
• mach.bat still uses a hardcoded version and runs vcvarsall.bat to set quite a few environmental variables. How is this code then required? Or put differently: Should mach.bat just rely on this code? I can only tell that locally running in mozilla-build, everything works, I use ./mach instead of mach.bat, though I cannot test mach.bat because it WOULD find vcvars from my 2015 install, which doesn't contain MSBuild though. Testing that would help me a lot.
• I've read somewhere that Servo requires the Graphics Workload, but I couldn't find that in the docs. Maybe someone knows this cleary? Because then we could add Microsoft.VisualStudio.Component.Graphics.Tools to vswhere, so that only Installations with the required workloads are considered.
• mozjs compilation fails [1] (I think this is unrelated to this change, relativesrcdir=mozpath.relpath(srcdir, obj.topsrcdir) or '.', -> This will always fail for me, as srcdir is ~/.cargo on C:\ and I've cloned servo to E:\

[1]:

Traceback (most recent call last):
  File "c:/Users/Marc/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/../../configure.py", line 132, in <module>
    sys.exit(main(sys.argv))
  File "c:/Users/Marc/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/../../configure.py", line 43, in main
    return config_status(config)
  File "c:/Users/Marc/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/5906588/mozjs/js/src/../../configure.py", line 127, in config_status
    return config_status(args=[], **encode(sanitized_config, encoding))
  File "c:\Users\Marc\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\5906588\mozjs\python\mozbuild\mozbuild\config_status.py", line 146, in config_status
    the_backend.consume(definitions)
  File "c:\Users\Marc\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\5906588\mozjs\python\mozbuild\mozbuild\backend\base.py", line 128, in consume
    if (not self.consume_object(obj) and
  File "c:\Users\Marc\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\5906588\mozjs\python\mozbuild\mozbuild\backend\recursivemake.py", line 438, in consume_object
    consumed = CommonBackend.consume_object(self, obj)
  File "c:\Users\Marc\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\5906588\mozjs\python\mozbuild\mozbuild\backend\common.py", line 134, in consume_object
    with self._get_preprocessor(obj) as pp:
  File "e:\mozilla-build\python\Lib\contextlib.py", line 17, in __enter__
    return self.gen.next()
  File "c:\Users\Marc\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\5906588\mozjs\python\mozbuild\mozbuild\backend\base.py", line 320, in _get_preprocessor
    srcdir_rel=mozpath.relpath(srcdir, mozpath.dirname(obj.output_path)),
  File "c:\Users\Marc\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\5906588\mozjs\python\mozbuild\mozpack\path.py", line 33, in relpath
    rel = normsep(os.path.relpath(path, start))
  File "e:\Marc\Documents\Coding\servo\target\debug\build\mozjs_sys-88004c1deb65babf\out\_virtualenvs\init\lib\ntpath.py", line 529, in relpath
    % (path_prefix, start_prefix))
ValueError: path is on drive c:, start on drive e:
mozmake: *** [makefile.cargo:197: maybe-configure] Error 1
thread 'main' panicked at 'assertion failed: result.success()', C:\Users\Marc\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\5906588\build.rs:133:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

[saschanaz]

I think the existing detection code isn't needed if vswhere is used. (👍)

[jdm]

The mozjs build failure happens when your CARGO_HOME directory is on a different drive than your servo working directory.

[jdm]

Let's say "Please set the VSINSTALLDIR and VisualStudioVersion environment variables." instead.

[jdm]

Let's write this as:

versions = sorted(vers, key=lambda tup: float(tup[1]))
if not versions:
    print(...)
    sys.exit(1)
return versions[0]

[MeFisto94]

Should I replace vers here by the direct function call?

[jdm]

I don't mind the way it is.

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit 0a80dd9 has been approved by jdm

[MeFisto94]

What about the two remaining things (which could be follow ups to this PR):

• Does servo require the graphics workload?
• mach.bat?

[jdm]

I am not aware of any requirement for the graphics workload. It's not clear to me what we can do differently with mach.bat.

[bors-servo]

⌛ Testing commit 0a80dd9 with merge 29d2ff1...

[saschanaz]

The mozjs build failure happens when your CARGO_HOME directory is on a different drive than your servo working directory.

Is this a reply for my comment? Not sure how relevant it is...

[jdm]

No, it's a reply to the part of the earlier comment that mentions a failure to build mozjs_sys.

[bors-servo]

💔 Test failed - status-taskcluster

[bors-servo]

💔 Test failed - status-taskcluster

[MeFisto94]

Speaking of mach.bat, in my opinion, we could delete everything before

servo/mach.bat

Line 49 in 79408fa

:mach

The reason is, that this searches for visual studio installations the old/legacy way and calls vcvars.bat.
I suspect this is so that MSBUILD is on PATH.
This will, however, fail early and thus not profit from the improved detection.
At the same time it's unnecessary to double-check for Visual Studio Installations.

In my experiments, having vcvars.bat excluded didn't break anything, especially since the code already locates the path to VS and uses that one (no need to have them on PATH, wouldn't work for me anyway [path limited to 1024 chars...]).

So we could do one of two things (or leave it as it is, of course):

• Do thorough testing and exclude the vcvars.bat and see if the build breaks under any condition for anyone in their specific environment
• Just continue with that legacy without thinking if it's required and just execute the .bat file from the python file (short after find_highest_msvc_version).

[jdm]

@bors-servo retry

[bors-servo]

⌛ Testing commit 0a80dd9 with merge 71e8aca...

[bors-servo]

💔 Test failed - status-taskcluster

[jdm]

@bors-servo retry

• network failure

[bors-servo]

⌛ Testing commit 0a80dd9 with merge 9f971f7...

[bors-servo]

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 9f971f7 to master...