Use Salt for mach bootstrap #14974

aneeshusa · 2017-01-12T01:13:45Z

This is currently WIP, but allows Salt for mach bootstrap. Not looking for review yet, just posting for visibility. You can run ./mach bootstrap and Salt will run, letting you know what changes it would make for bootstrapping (doesn't actually run yet though).

Currently, this reads from saltfs using gitfs, meaning it always tracks the master branch. (Note that this is blocked on servo/saltfs#577 landing; in the meantime, I've included a small workaround in this PR to pull from my updated saltfs branch, which will need to be removed.) In the future, the relevant Salt code may be merged in tree as part of Docker support for TC, and the bootstrapper should be updated to read from in tree.

Also, our Windows machines have not been Salted, so the existing Windows bootstrappers are retained.

TODO:

Hook into existing bootstrapping code less hackily
Actually bootstrap instead of always using test=True mode (includes sudo)
Default to interactive mode (test first, then ask), with a force flag
Don't require running from the repository root directory
Make it easy to add support for other distros

./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 they should be verified manually

This change is

highfive · 2017-01-12T01:13:49Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/requirements.txt, python/servo/bootstrapper/bootstrap.py, python/servo/bootstrapper/salt_config/minion, python/servo/bootstrapper/salt.py

aneeshusa · 2017-01-12T19:20:02Z

This should work on Ubuntu Trusty now.

@wafflespeanut @frewsxcv Python question: So far it seems necessary to use --system-site-packages when creating the virtualenv to allow Salt to work properly (see the commit message for details). Does this seem OK to you or should I make a separate virtualenv for Salt?


        Remove unusable --android flag for `mach bootstrap`

`mach` can't do any bootstrapping for Android, so the flag is useless.

aneeshusa · 2017-01-14T05:18:49Z

I've finished the remaining items on this, including a cleanup/overhaul of the existing bootstrapping. This is ready for review.

cc @UK992 since you wrote the original Windows bootstrapping, can you check that mach bootstrap still works on Windows?

aneeshusa · 2017-01-14T05:21:39Z

Note this is still blocked on servo/saltfs#577 landing, however.

wafflespeanut · 2017-01-14T05:54:06Z

python/servo/bootstrap.py

+        proceed = raw_input(
+            'Proposed changes are above, proceed with bootstrap? [y/N]: '
+        )
+        if proceed not in ['y', 'yes', 'Y', 'Yes', 'YES']:


if proceed.lower() not in ['y', 'yes']? 😄

wafflespeanut · 2017-01-14T05:54:42Z

This is great! 😄

Does this seem OK to you or should I make a separate virtualenv for Salt?

This should be fine, since pip install -I is going to reinstall our local requirements which will shadow the globally available libraries anyway.

aneeshusa · 2017-01-14T16:57:27Z

Also, I think this should work on macOS (using Homebrew) - if someone could check that I'll enable the Salt bootstrapper for macOS as well (instead of just Ubuntu 14.04).

UK992 · 2017-01-15T15:59:47Z

Since both gnu and msvc use same python, detection should be relied on host_triple.
For msvc, maybe could --force flag delete msvc-dependencies folder and reinstall all packages?
Also salt should be excluded in requirements.txt for Windows, because it's failed to create virtualenv.

UK992 · 2017-01-15T16:11:24Z

Also set() on WINDOWS_GNU packages list should be removed, because it fails to execute command.

aneeshusa · 2017-01-15T18:37:37Z

@UK992

I haven't changed the GNU/MSVC detection from the original. I'm not sure exactly how to use host_triple, can you show me how I should use it here?
We shouldn't never need to delete the msvc-dependencies folder, since each package version is installed in its folder and never changed, so IMO a --force flag is unnecessary. If we do want that behavior it's different from what --force does for the other two, so I think we would need two flags at that point.
Addressed your other two comments.

UK992 · 2017-01-15T19:14:46Z

GNU/MSVC detection was broken for some time. Using host_triple:

if "windows-gnu" in host_triple():
    bootstrapper = windows_gnu
elif "windows-msvc" in host_triple():
    bootstrapper = windows_msvc


        Create a util.py Python module for common functions

Extracting these functions helps avoid circular dependencies, and make them easier to find/reuse.


        Avoid hardcoded references to rustc in download()

Use the `desc` parameter instead to make the error messages customized for the actual download. Also use new-style format strings.


        Clean up and simplify existing `mach bootstrap`

- Default to interactive mode and remove the `--interactive` flag - Use `--force` to skip interactivity - Change MSVC dependency storage organization on disk: put each version into its own folder and directly refer to the versioned folders, providing immutability and making the installation list redundant - Reuse `host_triple()` function to fix broken bootstrapper dispatching - Simplify code: - Remove or inline many unused and redudant functions and variables - Prefer plain functions to classes - Consolidate into fewer files, remove unnecessary bootstrapper/ dir - Improve Python style - Sort dependency list

aneeshusa · 2017-01-15T21:02:57Z

@UK992 OK thanks, I updated that. Can you give it a try and also run ./mach build on MSVC after bootstrapping? I want to ensure the reorganizing of msvc-dependencies didn't break the build_env function.

UK992 · 2017-01-15T22:05:08Z

@aneeshusa building works perfectly on MSVC.


        Add a Salt bootstrapper

The Salt bootstrapper invokes Salt during `./mach bootstrap` to install Servo's build dependencies. It uses salt-call pinned to the same version of Salt as used in saltfs. Currently, the implementation uses gitfs and reads directly from the master branch of the saltfs repo; in the future this should be changed when the relevant Salt states are moved in-tree as part of Dockerization for TaskCluster. We have not Salted our Windows machines, so the existing Windows bootstrappers are retained. Currently this is only tested on Ubuntu Trusty. Salt uses various system python libraries, including `python-apt` on Debian-based OSes to interact with apt. `python-apt` does not seem to be installable via a requirements.txt file, and the versions available on PyPI are far behind the versions installed on actual Ubuntu machines. Additionally, adding `python-apt` as an unconditional python dependency would add bloat for users of other OSes, and lead to more churn as additional OSes are supported. However, as `python-apt` is already installed via apt on these machines, we can allow Salt to instead use the module by using `--system-site-packages` for the python virtualenv. We also add the `-I` flag to `pip install` to ensure we have a local, untouched copy of any other python packages used. However, because this prints system-level Python packages in scope, it slightly breaks isolation, so it is important to always pin all dependencies in the requirements files.

aneeshusa · 2017-01-17T16:10:52Z

servo/saltfs#577 merged and I've updated this, so this is no longer blocked.

jdm · 2017-01-19T21:57:29Z

@metajack Review ping!

metajack · 2017-01-20T04:58:16Z

@bors-servo r+

Looks good to me.

bors-servo · 2017-01-20T04:58:17Z

📌 Commit b4f5086 has been approved by metajack

bors-servo · 2017-01-20T04:58:20Z

⌛ Testing commit b4f5086 with merge 1f76aa6...


        Auto merge of #14974 - aneeshusa:add-mach-bootstrap, r=metajack

Use Salt for mach bootstrap  This is currently WIP, but allows Salt for `mach bootstrap`. Not looking for review yet, just posting for visibility. You can run `./mach bootstrap` and Salt will run, letting you know what changes it would make for bootstrapping (doesn't actually run yet though). Currently, this reads from saltfs using gitfs, meaning it always tracks the master branch. (Note that this is blocked on servo/saltfs#577 landing; in the meantime, I've included a small workaround in this PR to pull from my updated saltfs branch, which will need to be removed.) In the future, the relevant Salt code may be merged in tree as part of Docker support for TC, and the bootstrapper should be updated to read from in tree. Also, our Windows machines have not been Salted, so the existing Windows bootstrappers are retained. TODO: - [x] Hook into existing bootstrapping code less hackily - [x] Actually bootstrap instead of always using `test=True` mode (includes sudo) - [x] Default to interactive mode (test first, then ask), with a force flag - [x] Don't require running from the repository root directory - [x] Make it easy to add support for other distros ---  - [ ] `./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 they should be verified manually   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14974)

bors-servo · 2017-01-20T05:53:42Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned metajack Jan 12, 2017

highfive added the S-awaiting-review label Jan 12, 2017

aneeshusa mentioned this pull request Jan 12, 2017

Mach Bootstrap: Support for more platforms #13226

Closed

aneeshusa force-pushed the aneeshusa:add-mach-bootstrap branch 5 times, most recently from 1626266 to 0e0f9d7 Jan 12, 2017

aneeshusa added the S-awaiting-answer label Jan 12, 2017

aneeshusa force-pushed the aneeshusa:add-mach-bootstrap branch from 0e0f9d7 to f48f8e6 Jan 14, 2017

Remove unusable --android flag for `mach bootstrap`

5b8d783

`mach` can't do any bootstrapping for Android, so the flag is useless.

aneeshusa force-pushed the aneeshusa:add-mach-bootstrap branch 2 times, most recently from 84429a2 to 6b00847 Jan 14, 2017

aneeshusa changed the title ~~WIP: Use Salt for mach bootstrap~~ Use Salt for mach bootstrap Jan 14, 2017

aneeshusa added the S-blocked-on-external label Jan 14, 2017

wafflespeanut reviewed Jan 14, 2017

View changes

aneeshusa force-pushed the aneeshusa:add-mach-bootstrap branch from 6b00847 to 9b6e5e4 Jan 14, 2017

aneeshusa force-pushed the aneeshusa:add-mach-bootstrap branch 3 times, most recently from 0263adf to 966a2c4 Jan 15, 2017

aneeshusa added 3 commits Jan 14, 2017

Create a util.py Python module for common functions

02b054e

Extracting these functions helps avoid circular dependencies, and make them easier to find/reuse.

Avoid hardcoded references to rustc in download()

ef900cb

Use the `desc` parameter instead to make the error messages customized for the actual download. Also use new-style format strings.

aneeshusa force-pushed the aneeshusa:add-mach-bootstrap branch from 966a2c4 to 94dc386 Jan 15, 2017

aneeshusa force-pushed the aneeshusa:add-mach-bootstrap branch from 94dc386 to b4f5086 Jan 17, 2017

aneeshusa removed S-awaiting-answer S-blocked-on-external labels Jan 17, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 20, 2017

bors-servo merged commit b4f5086 into servo:master Jan 20, 2017
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 Jan 20, 2017

bors-servo mentioned this pull request Jan 20, 2017

[WIP] Exit if pip version is detected to be too old. #14253

Closed

3 of 5 tasks complete

servo / servo

Use Salt for mach bootstrap #14974

Use Salt for mach bootstrap #14974

aneeshusa commented Jan 12, 2017 •

edited

highfive commented Jan 12, 2017

aneeshusa commented Jan 12, 2017

aneeshusa commented Jan 14, 2017

aneeshusa commented Jan 14, 2017

This comment has been minimized.

wafflespeanut commented Jan 14, 2017

aneeshusa commented Jan 14, 2017

UK992 commented Jan 15, 2017 •

edited

UK992 commented Jan 15, 2017

aneeshusa commented Jan 15, 2017

UK992 commented Jan 15, 2017

aneeshusa commented Jan 15, 2017

UK992 commented Jan 15, 2017

aneeshusa commented Jan 17, 2017

jdm commented Jan 19, 2017

metajack commented Jan 20, 2017

bors-servo commented Jan 20, 2017

bors-servo commented Jan 20, 2017

bors-servo commented Jan 20, 2017

servo / servo

Join GitHub today

Use Salt for mach bootstrap #14974

Use Salt for mach bootstrap #14974

Conversation

aneeshusa commented Jan 12, 2017 • edited

highfive commented Jan 12, 2017

aneeshusa commented Jan 12, 2017

aneeshusa commented Jan 14, 2017

aneeshusa commented Jan 14, 2017

This comment has been minimized.

wafflespeanut Jan 14, 2017

wafflespeanut commented Jan 14, 2017

aneeshusa commented Jan 14, 2017

UK992 commented Jan 15, 2017 • edited

UK992 commented Jan 15, 2017

aneeshusa commented Jan 15, 2017

UK992 commented Jan 15, 2017

aneeshusa commented Jan 15, 2017

UK992 commented Jan 15, 2017

aneeshusa commented Jan 17, 2017

jdm commented Jan 19, 2017

metajack commented Jan 20, 2017

bors-servo commented Jan 20, 2017

bors-servo commented Jan 20, 2017

bors-servo commented Jan 20, 2017

aneeshusa commented Jan 12, 2017 •

edited

UK992 commented Jan 15, 2017 •

edited