-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Windows support for the --nightly | -n flag to mach run commands. #20122
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
Windows support for the --nightly | -n flag to mach run commands. #20122
Conversation
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon. |
Heads up! This PR modifies the following files:
|
336a10b
to
b97aa8e
Compare
I guess we should merge #19947, and then go to work on this. The two code are overlaid. |
Yeah definitely ! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good improvement on the original PR. Thanks!
python/servo/command_base.py
Outdated
print("Extracting to {} ...".format(destination_folder)) | ||
if is_windows(): | ||
command = 'msiexec /a {} /qn TARGETDIR={}' \ | ||
.format(os.path.join(nightlies_folder, destination_file), destination_folder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use this formatting instead:
command = 'msiexec /a {} /qn TARGETDIR={}'.format(
path.join(...), destination_folder)
python/servo/command_base.py
Outdated
bin_folder = path.join(destination_folder, "servo") | ||
if is_windows(): | ||
bin_folder = path.join(destination_folder, "PFiles", "Mozilla research", "Servo Tech Demo") | ||
print(path.join(bin_folder, "servo{}".format(BIN_SUFFIX))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I could extract the .msi archive to %temp% and just keep the .exe file instead :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was asking about the print
, but it's worth a try!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, no it's debug code I use (I seem to get the right path but when I try to ./mach run with a path to the executable it just won't ^^
I guess as soon as I figure it out I'll remove the WIP tag on the PR (Probably this weekend :))
☔ The latest upstream changes (presumably #19947) made this pull request unmergeable. Please resolve the merge conflicts. |
b97aa8e
to
887fcf6
Compare
887fcf6
to
a132a9b
Compare
This should be reviewable now. The command to run would be
|
Is there a difference if you run a windows nightly since March 1? There was a two month period in which we broke nightlies on windows at startup. |
I got the same result as I ran
The window seems to open and closes instantly.
This is definitely related to my VM setup x) |
Let's go ahead and merge this. If anybody tests it in a non-VM environment they can let us know how it goes. |
📌 Commit a132a9b has been approved by |
Windows support for the --nightly | -n flag to mach run commands. <!-- Please describe your changes on the following line: --> Add windows support to the -n flag. --- Followup to #19947 , this PR will add windows support to the -n flag. This is part of step two for #19505 <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). The feature does not work just yet, I'm able to download and extract the archive, but it's not running the executable yet. @tigercosmos might be a good reviewer on this one :) <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because they're part of a ./mach command. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20122) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev |
./mach run -n on mac os. <!-- Please describe your changes on the following line: --> Add macos support to the -n flag. Followup to #20122 , this PR will add macos support to the -n flag. This is part of step two for #19505 --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because this mach command does not build anything <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22387) <!-- Reviewable:end -->
Add windows support to the -n flag.
Followup to #19947 , this PR will add windows support to the -n flag.
This is part of step two for #19505
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThe feature does not work just yet, I'm able to download and extract the archive, but it's not running the executable yet.
@tigercosmos might be a good reviewer on this one :)
This change is