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: Add more platforms #579

Merged
merged 3 commits into from Jan 25, 2017
Merged

Conversation

@UK992
Copy link
Contributor

UK992 commented Jan 22, 2017

This PR add support for Fedora/CentOS platforms and also Ubuntu 16.04.

r? @aneeshusa


This change is Reviewable

@aneeshusa
Copy link
Member

aneeshusa commented Jan 22, 2017

I haven't reviewed this yet, but please rebase on top of #582 and make similar additions for other OSes. (The requirement is LLVM > 3.3, so exact version can vary a little.)

@UK992 UK992 force-pushed the UK992:bootstrap-salt branch from 49dcdc5 to 86b4612 Jan 22, 2017
@UK992
Copy link
Contributor Author

UK992 commented Jan 22, 2017

Done, llvm-devel installs llvm 3.8

@@ -21,7 +21,7 @@ python2-dev:
pip:
pkg.installed:
- pkgs:
{% if grains['os'] == 'Ubuntu' %}
{% if grains['os'] in ('CentOS', 'Fedora', 'Ubuntu') %}

This comment has been minimized.

@aneeshusa

aneeshusa Jan 23, 2017

Member

nit: use a list instead of a tuple

@@ -35,6 +35,31 @@ servo-dependencies:
- xpra
- xserver-xorg-input-void
- xserver-xorg-video-dummy
{% elif grains['os'] in ('CentOS', 'Fedora') %}

This comment has been minimized.

@aneeshusa

aneeshusa Jan 23, 2017

Member

nit: use a list instead of a tuple

- libXi-devel
- libXmu-devel
- libXrandr-devel
- libtool

This comment has been minimized.

@aneeshusa

aneeshusa Jan 23, 2017

Member

nit: for sorting, put libtool before the various libX* packages

- mesa-libGL-devel
- mesa-libOSMesa-devel
- openssl-devel
- rpm-build

This comment has been minimized.

@aneeshusa

aneeshusa Jan 23, 2017

Member

Why is this package needed?

This comment has been minimized.

@UK992

UK992 Jan 25, 2017

Author Contributor

this one is needed for osmesa

- freetype-devel
- gcc-c++
- glib2-devel
- gperf

This comment has been minimized.

@aneeshusa

aneeshusa Jan 23, 2017

Member

Why is this package needed?

This comment has been minimized.

@jdm

jdm Jan 25, 2017

Member

It's in the list of Servo prerequisites at https://github.com/servo/servo/#on-debian-based-linuxes; I forget which dependency in particular requires it.


{% if grains['os'] == 'Ubuntu' %}
{% if grains['os'] == 'Ubuntu' and not salt['pillar.get']('is_bootstrap') %}

This comment has been minimized.

@aneeshusa

aneeshusa Jan 23, 2017

Member

Why disable this? We need the multiverse repo for fonts IIRC.

This comment has been minimized.

@UK992

UK992 Jan 25, 2017

Author Contributor

now it's only enabled on Ubuntu 14

@@ -28,6 +28,7 @@ pip:
{% endif %}
- reload_modules: True

{% if not salt['pillar.get']('is_bootstrap') %}

This comment has been minimized.

@aneeshusa

aneeshusa Jan 23, 2017

Member

There's no is_bootstrap pillar; use {% if salt['pillar.get']('fully_managed', True) %} for now instead.

This comment has been minimized.

@aneeshusa

aneeshusa Jan 23, 2017

Member

Also, I don't see the harm in having this while bootstrapping anyways.

@@ -36,15 +36,17 @@ servo-dependencies:
- xserver-xorg-input-void
- xserver-xorg-video-dummy
{% endif %}
{% if not salt['pillar.get']('is_bootstrap') %}

This comment has been minimized.

@aneeshusa

aneeshusa Jan 23, 2017

Member

Ditto about using fully_managed for now.

@aneeshusa
Copy link
Member

aneeshusa commented Jan 23, 2017

Overall looks good, but I'd like to take this chance to remove any obsolete dependencies. Also, there is no is_bootstrap pillar yet; I'll need to think a little more about pillar items to use and refactoring to handle bootstrapping, our prod infra, Dockerfiles, etc.

@UK992 UK992 force-pushed the UK992:bootstrap-salt branch from 86b4612 to 4dc6227 Jan 25, 2017

{% if grains['os'] == 'Ubuntu' %}
{% if grains['os'] == 'Ubuntu' and grains['oscodename'] == 'trusty' %}
multiverse:
pkgrepo.managed:
- name: 'deb http://archive.ubuntu.com/ubuntu trusty multiverse'

This comment has been minimized.

@aneeshusa

aneeshusa Jan 25, 2017

Member

Instead of gating on being Trusty, try replacing trusty on this line with {{ grains['oscodename'] }}. I'm not 100% sure but I think it should work on Trusty and Xenial both.

This comment has been minimized.

@UK992

UK992 Jan 25, 2017

Author Contributor

Doesn't work on Xenial.

This comment has been minimized.

@aneeshusa

aneeshusa Jan 25, 2017

Member

I just tried it locally with an ubuntu/xenial64 Vagrant box and it worked fine. What error are you getting?

This comment has been minimized.

@UK992

UK992 Jan 25, 2017

Author Contributor

that python-apt is not installed, after installing it, still same error.

This comment has been minimized.

@UK992

UK992 Jan 25, 2017

Author Contributor
[ERROR   ] Failed to examine repo 'deb http://archive.ubuntu.com/ubuntu xenial multiverse': Error: 'python-apt' package not installed
[ERROR   ] State 'debconf.set' was not found in SLS 'servo-build-dependencies'
Reason: 'debconf.set' is not available.

This comment has been minimized.

@aneeshusa

aneeshusa Jan 25, 2017

Member

OK, I've reproduced this and the fix is non-trivial so I will handle it in a follow up.

@aneeshusa
Copy link
Member

aneeshusa commented Jan 25, 2017

Thanks @UK992! @bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jan 25, 2017

📌 Commit 4dc6227 has been approved by aneeshusa

@bors-servo
Copy link
Contributor

bors-servo commented Jan 25, 2017

Test exempted - status

@bors-servo bors-servo merged commit 4dc6227 into servo:master Jan 25, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test exempted
Details
bors-servo added a commit that referenced this pull request Jan 25, 2017
Mach bootstrap: Add more platforms

This PR add support for Fedora/CentOS platforms and also Ubuntu 16.04.

r? @aneeshusa

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/579)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.