Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mach.bat: Remove manual Visual Studio detection and defer to command_base.py #25336

Closed
MeFisto94 opened this issue Dec 18, 2019 · 3 comments
Closed

mach.bat: Remove manual Visual Studio detection and defer to command_base.py #25336

MeFisto94 opened this issue Dec 18, 2019 · 3 comments
Labels
A-mach P-windows

Comments

@MeFisto94
Copy link
Contributor

@MeFisto94 MeFisto94 commented Dec 18, 2019

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).

Originally posted by @MeFisto94 in #25300 (comment)
@highfive
Copy link

@highfive highfive commented Dec 18, 2019

cc @wafflespeanut
@jdm
Copy link
Member

@jdm jdm commented Dec 18, 2019

A good first start would be to see if switching to python mach instead of mach in https://github.com/servo/servo/blob/master/etc/taskcluster/decision_task.py#L487-L496 passes CI.
This was referenced Dec 22, 2019
Closed
Merged
bors-servo added a commit that referenced this issue Dec 23, 2019
@bors-servo
Auto merge of #25365 - MeFisto94:autolaunch-vcvars, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
3c2deaf 
Launch vcvarsall.bat for the recognized VS Installation Directory from python instead of making mach.bat try that on hardcoded paths.

Move the Execution of vcvars (which sets up the environment for visual studio tools) from mach.bat to python, so that ./mach works under mozilla-build and that #25300 can be used.
---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25360 #25336
- [X] These changes do not require tests because changes to build infra
bors-servo added a commit that referenced this issue Jan 9, 2020
@bors-servo
Auto merge of #25365 - MeFisto94:autolaunch-vcvars, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
5c3b061 
Launch vcvarsall.bat for the recognized VS Installation Directory from python instead of making mach.bat try that on hardcoded paths.

Move the Execution of vcvars (which sets up the environment for visual studio tools) from mach.bat to python, so that ./mach works under mozilla-build and that #25300 can be used.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25360 #25336
- [X] These changes do not require tests because changes to build infra
bors-servo added a commit that referenced this issue Mar 24, 2020
@bors-servo
Auto merge of #25365 - MeFisto94:autolaunch-vcvars, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
d1c76ed 
Launch vcvarsall.bat for the recognized VS Installation Directory from python instead of making mach.bat try that on hardcoded paths.

Move the Execution of vcvars (which sets up the environment for visual studio tools) from mach.bat to python, so that ./mach works under mozilla-build and that #25300 can be used.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25360 #25336
- [X] These changes do not require tests because changes to build infra
@atouchet
Copy link
Contributor

@atouchet atouchet commented Mar 24, 2020

This was done in #25365.
@atouchet atouchet closed this Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-mach P-windows
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
@jdm @highfive @MeFisto94 @atouchet
You can’t perform that action at this time.