Skip to content
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

Mach bootstrap: Improve and support more platforms #15230

Merged
merged 1 commit into from Mar 4, 2017

Conversation

Projects
None yet
6 participants
@UK992
Copy link
Contributor

commented Jan 26, 2017

r? @aneeshusa or @wafflespeanut


  • ./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 _____

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jan 26, 2017

Heads up! This PR modifies the following files:

@aneeshusa
Copy link
Member

left a comment

Core concept is good, but many of the additions have dubious usefulness IMO.

@@ -219,7 +247,8 @@ def bootstrap(context, force=False):
bootstrapper = windows_msvc
elif "linux-gnu" in host_triple():
distro, version, _ = platform.linux_distribution()
if distro == 'Ubuntu' and version == '14.04':
if distro in ('CentOS', 'CentOS Linux', 'Fedora', 'Ubuntu'):

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Jan 26, 2017

Member

After bootstrapping, is it possible to build Servo even without the MS fonts?

This comment has been minimized.

Copy link
@UK992

UK992 Jan 26, 2017

Author Contributor

Yes, it's possible, tested on Ubuntu 16.04.

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Feb 7, 2017

Member

Same comment about using a set or list here instead of a tuple.

return 1
print("done")
virtualenv_script_dir = os.path.join(context.topdir, 'python', '_virtualenv', _get_virtualenv_script_dir())
if not os.path.exists(os.path.join(virtualenv_script_dir, 'salt-call')):

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Jan 26, 2017

Member

This is not quite right, since from time to time we will need to update Salt to stay in locksetep with saltfs. I'd prefer to continue running pip install every time, which is a (fast) no-op if the dependencies are up to date.

This comment has been minimized.

Copy link
@UK992

UK992 Jan 26, 2017

Author Contributor

I reverted this change.

print('Output: {}\nError: {}'.format(out, err))
return 1
print("done")
virtualenv_script_dir = os.path.join(context.topdir, 'python', '_virtualenv', _get_virtualenv_script_dir())

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Jan 26, 2017

Member

It would be nice to add an absolute=True (optional, kwarg?) arg to _get_virtualenv_script_dir so thatthe path prefixes don't need to be appended here and in mach_bootstrap.py.

if process.returncode:
out, err = process.communicate()
if "pycrypto/setup.py" in err and not retry:
return install_salt_dependencies(context, force)

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Jan 26, 2017

Member

I think it makes more sense to do this before running pip install, to avoid having to call pip install twice.

This comment has been minimized.

Copy link
@UK992

UK992 Jan 26, 2017

Author Contributor

since install_salt_dependencies only run, when failed to install pycrypto dependency of Salt, probably one time run, i think it's better run pip install twice, then install_salt_dependencies on every run.

subprocess.check_call(['sudo', '-v'], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
command.insert(0, 'sudo')
except:
command = ['su', 'root', '-c', ' '.join(command)]

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Jan 26, 2017

Member

Where is su needed? I much prefer using sudo in all circumstances, since su 1) uses root's environment instead of the user's environment and 2) requires the root password instead of the user's password.

This comment has been minimized.

Copy link
@UK992

UK992 Jan 26, 2017

Author Contributor

On Fedora with user account without password, sudoing is not possible.

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Feb 7, 2017

Member

IMO that's an insecure/broken setup (e.g. another sudo benefit is its auditing support), and I'd rather not support it. Additionally, the su codepath does not handle quoting/escaping of the command that it passes to bash, which is also easy to write incorrectly. Users with such setups can always install dependencies manually.

Also, in case a user uses sudo with timestamp_timeout set to 0, the additional sudo -v call means they will have twice as many sudo prompts.

return subprocess.call(command)


# This install needed dependencies for Salt

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Jan 26, 2017

Member

nit: remove, comment is redundant with function name

@wafflespeanut

This comment has been minimized.

Copy link
Member

commented Feb 4, 2017

Review ping @aneeshusa

@aneeshusa
Copy link
Member

left a comment

Looking better, thanks for this PR! I have some more review comments.

@@ -96,11 +96,12 @@ def _get_exec_path(names, is_valid_path=lambda _path: True):
return None


def _get_virtualenv_script_dir():
def _get_virtualenv_script_dir(absolute=True):

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Feb 7, 2017

Member

We're no longer using this for the Salt bootstrap, so let's back out the change to this function and its call site in _activate_virtualenv.

subprocess.check_call(['sudo', '-v'], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
command.insert(0, 'sudo')
except:
command = ['su', 'root', '-c', ' '.join(command)]

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Feb 7, 2017

Member

IMO that's an insecure/broken setup (e.g. another sudo benefit is its auditing support), and I'd rather not support it. Additionally, the su codepath does not handle quoting/escaping of the command that it passes to bash, which is also easy to write incorrectly. Users with such setups can always install dependencies manually.

Also, in case a user uses sudo with timestamp_timeout set to 0, the additional sudo -v call means they will have twice as many sudo prompts.

print("Installing missing Salt dependencies...")
if context.distro == 'Ubuntu':
run_as_root(['apt-get', 'install', '-y', 'build-essential', 'libssl-dev', 'libffi-dev', 'python-dev'])
elif context.distro in ('CentOS', 'CentOS Linux', 'Fedora'):

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Feb 7, 2017

Member

Use a set or list instead of a tuple.

@@ -219,7 +247,8 @@ def bootstrap(context, force=False):
bootstrapper = windows_msvc
elif "linux-gnu" in host_triple():
distro, version, _ = platform.linux_distribution()
if distro == 'Ubuntu' and version == '14.04':
if distro in ('CentOS', 'CentOS Linux', 'Fedora', 'Ubuntu'):

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Feb 7, 2017

Member

Same comment about using a set or list here instead of a tuple.

return subprocess.call(command)


def install_salt_dependencies(context, force):

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Feb 7, 2017

Member

This method should use the force parameter as it is used elsewhere: if force is False, then it should prompt the user before installing packages.

@@ -29,6 +51,9 @@ def salt(context, force=False):
process.wait()
if process.returncode:
out, err = process.communicate()
if "pycrypto/setup.py" in err and not retry:
return install_salt_dependencies(context, force)

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Feb 7, 2017

Member

Hmm, I don't know where my previous comment about this went.

Checking for a substring of the error message in this way seems fragile to me; Salt has changed crypto libraries in the past.

The mutual recursion here is error-prone as well; if in the future there is a new system-level dependency for Salt, then this code won't install it, causing the error on each Salt invocation and endlessly recursing through salt and install_salt_dependencies in a loop, causing the process to hang.

I still think it's better to check for the the Salt dependencies on each run before installing Salt. We can make this faster by using dpkg -s <package> <names> <here> first, and only running apt-get install if it returns nonzero; I'm not sure about the CentOS/Fedora equivalent.

@UK992 UK992 force-pushed the UK992:mach-bootstrap branch from 5c54f6a to ab98e8c Feb 7, 2017

@UK992 UK992 force-pushed the UK992:mach-bootstrap branch 2 times, most recently from 439f9e1 to 226f16a Feb 7, 2017

@UK992 UK992 force-pushed the UK992:mach-bootstrap branch from 226f16a to f21d448 Feb 8, 2017

@UK992

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2017

All comments addressed!

@jdm

This comment has been minimized.

Copy link
Member

commented Feb 27, 2017

@aneeshusa Review ping!

@wafflespeanut

This comment has been minimized.

Copy link
Member

commented Mar 4, 2017

This has been around for way too long. I'd better land this.

@bors-servo r=aneeshusa

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2017

📌 Commit f21d448 has been approved by aneeshusa

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2017

⌛️ Testing commit f21d448 with merge d8a8e3e...

bors-servo added a commit that referenced this pull request Mar 4, 2017

Auto merge of #15230 - UK992:mach-bootstrap, r=aneeshusa
Mach bootstrap: Improve and support more platforms

<!-- Please describe your changes on the following line: -->
r? @aneeshusa or @wafflespeanut

---
<!-- 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
- [ ] These changes do not require tests because _____

<!-- 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/15230)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2017

☀️ 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
Approved by: aneeshusa
Pushing d8a8e3e to master...

@bors-servo bors-servo merged commit f21d448 into servo:master Mar 4, 2017

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.