Add a --nightly | -n flag to mach run commands for linux #19947

o0Ignition0o · 2018-02-04T16:27:10Z

First tries to download and extract a specific nightly version to run mach commands against.

I chose to split the Pull requests for each platform to avoid submitting a huge one, and to make sure I get the logic right.
I'm able to download / extract a nightly version, and I keep nightly versions in the target folder.
Windows and Mac OS support will be filed in separate PRs.
This is part of step two for #19505

The mentor on the issue is jdm

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because it is part of a ./mach command.

This change is

highfive · 2018-02-04T16:27:17Z

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

highfive · 2018-02-04T16:27:17Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/servo/command_base.py, python/servo/post_build_commands.py

tigercosmos · 2018-02-05T08:38:46Z

python/servo/command_base.py

+        if is_windows():
+            print("The nightly flag is not supported on windows yet.")
+            sys.exit(1)
+            os_prefix = "windows-msvc"


why we change os_prefix after the program has exited?

Good catch ! I was actually working on a multi platform solution, and then decided to split the merge requests. I'll remove it :)

bors-servo · 2018-02-13T14:55:06Z

☔ The latest upstream changes (presumably #19975) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2018-02-25T23:04:52Z

r? @jdm

Sorry about the delay; let's get a more appropriate reviewer on this.

o0Ignition0o · 2018-02-26T05:53:59Z

Hi :)
No Problem, i've started working on the windows support since then. I'll put it in a separate PR :)

o0Ignition0o · 2018-02-26T15:02:12Z

Build FAILED in 0:00:00

This looks weird, but I'm not really sure it's related to the changes I've made, any thoughts ?

jdm · 2018-02-26T15:20:37Z

You can ignore the taskcluster failure.

o0Ignition0o · 2018-02-26T15:22:09Z

Ok thanks !
I guess it should be reviewable then :)

o0Ignition0o · 2018-03-05T17:35:57Z

Hey there :) is anyone available for some suggestions or a code review ? I'm not at home but I'll ask on IRC as soon as I come home :)

jdm · 2018-03-05T17:53:04Z

Ugh, sorry for forgetting about this. I'll review it today.

jdm

This looks really great! Thanks!

jdm · 2018-03-05T18:00:25Z

python/servo/post_build_commands.py

        env = self.build_env()
        env["RUST_BACKTRACE"] = "1"

-        servo_cmd = [bin or self.get_binary_path(release, dev)] + params
+        servo_cmd = [bin or self.get_nightly_binary_path(
+            nightly.strip()) or self.get_binary_path(release, dev)] + params


nit: let's join these lines, or split after the or instead.

jdm · 2018-03-05T18:00:25Z

python/servo/command_base.py

+                e.reason))
+            sys.exit(1)
+        except AttributeError as e:
+            print("Could not fetch a nightly version for date {} and platform {} ".format(


nit: remove the extra space after {}

jdm · 2018-03-05T18:00:25Z

python/servo/command_base.py

+        # (eg /foo/bar/baz.tar.gz extracts to /foo/bar/baz)
+        destination_file = path.join(
+            nightly_target_directory, file_to_download)
+        destination_folder = destination_file[:destination_file.index('.')]


We can use os.path.splitext(destination_file)[0] instead.

I didn't know about this method, pretty cool !


        Auto merge of #19947 - o0Ignition0o:mach_run_nightly, r=jdm

Add a --nightly | -n flag to mach run commands for linux First tries to download and extract a specific nightly version to run mach commands against.  I chose to split the Pull requests for each platform to avoid submitting a huge one, and to make sure I get the logic right. I'm able to download / extract a nightly version, and I keep nightly versions in the target folder. Windows and Mac OS support will be filed in separate PRs. This is part of step two for #19505 The mentor on the issue is jdm ---  - [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).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because it is part of a ./mach command.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19947)

bors-servo · 2018-03-06T20:31:29Z

💔 Test failed - mac-rel-wpt1

jdm · 2018-03-06T20:37:50Z

Error running mach:

    ['run', '-r', '-o', 'output.png']

The error occurred in the implementation of the invoked mach command.

This should never occur and is likely a bug in the implementation of that
command. Consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

AttributeError: 'NoneType' object has no attribute 'strip'

  File "/Users/servo/buildbot/slave/mac-rel-wpt1/build/python/servo/post_build_commands.py", line 100, in run
    args = [bin or self.get_nightly_binary_path(nightly.strip()) or self.get_binary_path(release, dev)]

o0Ignition0o · 2018-03-06T20:52:47Z

Ok I found two ways to handle this issue:
I can use an if / else construct to check if the string is empty or not.
I can change the command argument to default to "", but that would break the "command arguments consistency" eg :

@CommandArgument('--debugger', default=None, type=str, help='Name of debugger to use.') @CommandArgument('--headless', '-z', action='store_true', help='Launch in headless mode') @CommandArgument('--software', '-s', action='store_true', help='Launch with software rendering') @CommandArgument('--bin', default=None, help='Launch with specific binary') @CommandArgument('--nightly', '-n', default="", help='Specify a YYYY-MM-DD nightly build to run') @CommandArgument( 'params', nargs='...', help="Command-line arguments to be passed through to Servo") def run(self, params, release=False, dev=False, android=None, debug=False, debugger=None, headless=False, software=False, bin=None, nightly=""):

I think using "" as default would be more elegant, but I can understand that CommandArgument / public api consistency might be more important.
Which one would you rather have?

o0Ignition0o · 2018-03-06T21:51:23Z

Ok I went for the empty default string, please let me know if you think the other approach is better :)

jdm · 2018-03-06T22:02:00Z

@bors-servo try

bors-servo · 2018-03-06T22:02:06Z

⌛ Trying commit 67a7a26 with merge d4cfba5...


        Auto merge of #19947 - o0Ignition0o:mach_run_nightly, r=<try>

Add a --nightly | -n flag to mach run commands for linux First tries to download and extract a specific nightly version to run mach commands against.  I chose to split the Pull requests for each platform to avoid submitting a huge one, and to make sure I get the logic right. I'm able to download / extract a nightly version, and I keep nightly versions in the target folder. Windows and Mac OS support will be filed in separate PRs. This is part of step two for #19505 The mentor on the issue is jdm ---  - [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).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because it is part of a ./mach command.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19947)

Eijebong · 2018-03-06T22:02:12Z

@bors-servo try

jdm · 2018-03-06T22:04:05Z

@o0Ignition0o Another option would be to strip the value inside get_nightly_binary_path once it has been checked for None.


        Add a --nightly | -n flag to download and extract a specific nightly …

…version to run mach commands against. This currently only work on linux, but windows and mac os support will follow in subsequent PRs.

o0Ignition0o · 2018-03-06T22:11:55Z

It is indeed a better approach !

jdm · 2018-03-07T04:47:04Z

@bors-servo r+

bors-servo · 2018-03-07T04:47:05Z

📌 Commit 10822ce has been approved by jdm

bors-servo · 2018-03-07T04:47:10Z

⌛ Testing commit 10822ce with merge 9eb4175...


        Auto merge of #19947 - o0Ignition0o:mach_run_nightly, r=jdm

Add a --nightly | -n flag to mach run commands for linux First tries to download and extract a specific nightly version to run mach commands against.  I chose to split the Pull requests for each platform to avoid submitting a huge one, and to make sure I get the logic right. I'm able to download / extract a nightly version, and I keep nightly versions in the target folder. Windows and Mac OS support will be filed in separate PRs. This is part of step two for #19505 The mentor on the issue is jdm ---  - [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).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because it is part of a ./mach command.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19947)

bors-servo · 2018-03-07T05:49:20Z

☀️ 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
Approved by: jdm
Pushing 9eb4175 to master...


        Auto merge of #20122 - o0Ignition0o:mach_run_nightly_windows, r=jdm

@tigercosmos

Windows support for the --nightly | -n flag to mach run commands.  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  - [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 :)  - [ ] There are tests for these changes OR - [X] These changes do not require tests because they're part of a ./mach command.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20122)

highfive assigned glennw Feb 4, 2018

highfive added the S-awaiting-review label Feb 4, 2018

o0Ignition0o changed the title ~~[WIP] Add a --nightly | -n flag to mach commands~~ [WIP] Add a --nightly | -n flag to mach run commands Feb 4, 2018

o0Ignition0o force-pushed the o0Ignition0o:mach_run_nightly branch 6 times, most recently from d19f510 to e81328f Feb 4, 2018

o0Ignition0o changed the title ~~[WIP] Add a --nightly | -n flag to mach run commands~~ Add a --nightly | -n flag to mach run commands for linux Feb 4, 2018

tigercosmos reviewed Feb 5, 2018

View changes

o0Ignition0o force-pushed the o0Ignition0o:mach_run_nightly branch from e81328f to e115b5a Feb 5, 2018

highfive added the S-needs-rebase label Feb 13, 2018

o0Ignition0o force-pushed the o0Ignition0o:mach_run_nightly branch from e115b5a to ed3911b Feb 25, 2018

highfive assigned jdm and unassigned glennw Feb 25, 2018

o0Ignition0o force-pushed the o0Ignition0o:mach_run_nightly branch from ca0721a to ed3911b Feb 26, 2018

o0Ignition0o mentioned this pull request Feb 26, 2018

Windows support for the --nightly | -n flag to mach run commands. #20122

Merged

3 of 5 tasks complete

jdm requested changes Mar 5, 2018

View changes

jdm added the S-needs-code-changes label Mar 5, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 6, 2018

o0Ignition0o force-pushed the o0Ignition0o:mach_run_nightly branch from bea6150 to 67a7a26 Mar 6, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Mar 6, 2018

o0Ignition0o force-pushed the o0Ignition0o:mach_run_nightly branch from 67a7a26 to aa7b43b Mar 6, 2018

Add a --nightly | -n flag to download and extract a specific nightly …

Loading status checks…

10822ce

…version to run mach commands against. This currently only work on linux, but windows and mac os support will follow in subsequent PRs.

o0Ignition0o force-pushed the o0Ignition0o:mach_run_nightly branch from aa7b43b to 10822ce Mar 6, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 7, 2018

bors-servo merged commit 10822ce into servo:master Mar 7, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Mar 7, 2018

o0Ignition0o deleted the o0Ignition0o:mach_run_nightly branch Dec 8, 2018

servo / servo

Add a --nightly | -n flag to mach run commands for linux #19947

Add a --nightly | -n flag to mach run commands for linux #19947

o0Ignition0o commented Feb 4, 2018 •

edited by jdm

highfive commented Feb 4, 2018

highfive commented Feb 4, 2018

This comment has been minimized.

This comment has been minimized.

bors-servo commented Feb 13, 2018

jdm commented Feb 25, 2018

o0Ignition0o commented Feb 26, 2018

o0Ignition0o commented Feb 26, 2018

jdm commented Feb 26, 2018

o0Ignition0o commented Feb 26, 2018

o0Ignition0o commented Mar 5, 2018

jdm commented Mar 5, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Mar 6, 2018

jdm commented Mar 6, 2018

o0Ignition0o commented Mar 6, 2018

o0Ignition0o commented Mar 6, 2018

jdm commented Mar 6, 2018

bors-servo commented Mar 6, 2018

Eijebong commented Mar 6, 2018

jdm commented Mar 6, 2018

o0Ignition0o commented Mar 6, 2018

jdm commented Mar 7, 2018

bors-servo commented Mar 7, 2018

bors-servo commented Mar 7, 2018

bors-servo commented Mar 7, 2018

servo / servo

Join GitHub today

Add a --nightly | -n flag to mach run commands for linux #19947

Add a --nightly | -n flag to mach run commands for linux #19947

Conversation

o0Ignition0o commented Feb 4, 2018 • edited by jdm

highfive commented Feb 4, 2018

highfive commented Feb 4, 2018

This comment has been minimized.

tigercosmos Feb 5, 2018

This comment has been minimized.

o0Ignition0o Feb 5, 2018

bors-servo commented Feb 13, 2018

jdm commented Feb 25, 2018

o0Ignition0o commented Feb 26, 2018

o0Ignition0o commented Feb 26, 2018

jdm commented Feb 26, 2018

o0Ignition0o commented Feb 26, 2018

o0Ignition0o commented Mar 5, 2018

jdm commented Mar 5, 2018

jdm left a comment

This comment has been minimized.

jdm Mar 5, 2018

This comment has been minimized.

jdm Mar 5, 2018

This comment has been minimized.

jdm Mar 5, 2018

This comment has been minimized.

o0Ignition0o Mar 6, 2018

bors-servo commented Mar 6, 2018

jdm commented Mar 6, 2018

o0Ignition0o commented Mar 6, 2018

o0Ignition0o commented Mar 6, 2018

jdm commented Mar 6, 2018

bors-servo commented Mar 6, 2018

Eijebong commented Mar 6, 2018

jdm commented Mar 6, 2018

o0Ignition0o commented Mar 6, 2018

jdm commented Mar 7, 2018

bors-servo commented Mar 7, 2018

bors-servo commented Mar 7, 2018

bors-servo commented Mar 7, 2018

o0Ignition0o commented Feb 4, 2018 •

edited by jdm