959005 - rust needs newer xcode to build on servo build slaves

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Jack Moffitt [:jack]]

There is apparently some but in ar that fails with Rust's new static linking. See here for a failed build: http://servo-buildbot.pub.build.mozilla.org/builders/mac/builds/740/steps/shell/logs/stdio

The Xcode version is 4.1. Rust itself is tested on Lion, but using much newer Xcode. Can we get a newer version on our build slaves?

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

Yes, that should be possible but will require some repackaging of XCode and re-arranging of the puppet manifests (to support older XCode on non-servo machines). Is this work really required at blocker urgency ? Perhaps you can delay the static linking until Ben can take a look at this ?

Comment 2

[Jack Moffitt [:jack]]

We've basically had to close the tree for this rust upgrade, and this is now blocking that work landing. I can look for a workaround, but if I can't find one, we're basically stuck unless we just turn off the mac builders.

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

Either that or backing out the rust upgrade until the mac builders have been updated. What would you prefer ?

Comment 4

[Jack Moffitt [:jack]]

I found a workaround patch for the Rust compiler so that we could land this. Changing the priority down.

Comment 5

[[e-mail] (:bhearsum)]

Sorry - this bug got lost in my queue :(. I'm glad you found a workaround.

As Nick said, we need to do some repackaging of Xcode to make this work at all. We also need to figure out if we can upgrade the non-Servo build machines, or decide to splinter the Puppet manifests (currently they're 100% identical between Servo and non-Servo machines). Finally, we need to test to see if an upgrade actually works through Puppet - I don't recall us ever doing that before.

I'm not sure if I'll be the one that ends up looking at this - anyone with sufficient Puppet knowledge should be able to take it on.

Comment 6

[Jack Moffitt [:jack]]

We just hit this again as the LLVM Rust needs requires a newer Clang. This is blocking us from landing the recent Rust upgrade.

Comment 7

[[e-mail] (:bhearsum)]

I had some time today and started to look into this. Our build machines are Lion, and the newest supported Xcode for that is 4.6.3. Jack, is that new enough? If you're unsure we can try installing it on one of the machines and do a build.

I still can't make this my #1 priority, but as long as we don't need Xcode 5 I think I can prod it along.

Comment 8

[[e-mail] (:bhearsum)]

According to https://github.com/mozilla/rust, Rust needs clang 3. I unpacked the latest version of the command line tools, and it has "Apple LLVM version 4.2 (clang-425.0.28) (based on LLVM 3.2svn)" - that's promising. I'm going to install the tools on servo-lion-r5-001 and try a build.

Comment 9

[[e-mail] (:bhearsum)]

Attachment: make 10.7 xcode command line tools available

Dustin, the tl;dr here is that we need a newer compiler on the Servo machines. I'm currently testing out the latest Xcode command line tools available for Lion, and they seem to be working. Any tips on how to make the Servo machines install them, but not the general RelEng ones? Maybe something like put xcode_version and xcode_cmdline_tools_version in *-config.pp?

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8426342 [details] [diff] [review]
make 10.7 xcode command line tools available

I don't think you'll need to do anything.  This just adds the ability to install xcode command line tools on Lion, which won't interfere with anything for releng.  Admittedly they're different versions, but our xcode versions are all over the map anyway.

Comment 11

[[e-mail] (:bhearsum)]

OK, the build succeeded. So I just need to figure out how to get the xcode command line tools installed only on the Servo machines through Puppet.

(In reply to Dustin J. Mitchell [:dustin] from comment #10)
> Comment on attachment 8426342 [details] [diff] [review]
> make 10.7 xcode command line tools available
> 
> I don't think you'll need to do anything.  This just adds the ability to
> install xcode command line tools on Lion, which won't interfere with
> anything for releng.  Admittedly they're different versions, but our xcode
> versions are all over the map anyway.

I'm pretty sure I need to add an "packages::xcode_cmdline_tools" somewhere. The simplest way would be to add it to the servo build machine definitions - but I know that violates design choices of PuppetAgain. The next step down is toplevel::slave::build::standard, but that's shared by the releng build machines - hence my previous question.

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

(from irc)

Yes, you're right -- releng builders are currently using packages::xcode, so adding 'include packages::xcode_cmdline_tools' would not go well.

Really we're talking about different versions of XCode here, rather than the difference between cmdline_tools and the full install.  Right now, different orgs want different versions, but there's no reason to assume that a single org wouldn't want to install different versions of xcode on different hosts within the org.

I think that what you suggested in irc -- a config option for each org to specify its xcode versions -- is the most sensible.  At the same time, we should roll packages::xcode and packages::xcode_cmdline_tools into a single module.

In moco-config.pp, that might look like

  $xcode_version = $::macosx_productversion_major ? {
    10.6 => "4.2",
    10.7 => "4.1",
    10.8 => "4.5cli",
    10.9 => "4.1",
    default => "unkonwn"
  }

Comment 13

[[e-mail] (:bhearsum)]

Attachment: specify xcode versions

This doesn't go as far as integrating the command line tools and xcode into one package, but it does specify the versions for both. I need to test still to make sure this gets all the versions right - is this conceptually ok though?

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8427063 [details] [diff] [review]
specify xcode versions

Review of attachment 8427063 [details] [diff] [review]:
-----------------------------------------------------------------

Minor suggestions, but I think this will work fine.

::: manifests/moco-config.pp
@@ +253,5 @@
> +    $xcode_cmdline_tools_version = $::macosx_productversion_major ? {
> +        10.8 => "4.5",
> +        10.9 => "5.0",
> +        default => undef
> +    }

These need to be added to config::base as well

::: modules/packages/manifests/xcode.pp
@@ +35,2 @@
>  
> +                "4.1": {

Why verify that 4.2 only installs on 10.6, but install 4.1 anywhere?

It might make sense to remove the OS X version checks everywhere, and just add a comment for where it's known to work.  Then if we try to run a new combination and it just works, yay; otherwise, we'll have a comment indicating it wasn't known to work anyway.

::: modules/packages/manifests/xcode_cmdline_tools.pp
@@ +14,5 @@
>          Darwin: {
>              case $::macosx_productversion_major {
> +                10.7: {
> +                    case $::config::xcode_cmdline_tools_version {
> +                        "4.6.2":

This inverts the checks -- in xcode.pp, the outer case is for the xcode version, with the inner case for the OS X version.  Here in xcode_cmdline_tools.pp it's the opposite.  That should probably get cleaned up.

Comment 15

[[e-mail] (:bhearsum)]

(In reply to Dustin J. Mitchell [:dustin] from comment #14)
> ::: modules/packages/manifests/xcode.pp
> @@ +35,2 @@
> >  
> > +                "4.1": {
> 
> Why verify that 4.2 only installs on 10.6, but install 4.1 anywhere?
> 
> It might make sense to remove the OS X version checks everywhere, and just
> add a comment for where it's known to work.  Then if we try to run a new
> combination and it just works, yay; otherwise, we'll have a comment
> indicating it wasn't known to work anyway.

Per the existing comments, the version of 4.2 that we have only works on 10.6. The version of 4.1 that we have has no restrictions AFAIK. (And actually, I should remove the comment that says it doesn't on 10.9, because it clearly does since we're installing it there...)

> ::: modules/packages/manifests/xcode_cmdline_tools.pp
> @@ +14,5 @@
> >          Darwin: {
> >              case $::macosx_productversion_major {
> > +                10.7: {
> > +                    case $::config::xcode_cmdline_tools_version {
> > +                        "4.6.2":
> 
> This inverts the checks -- in xcode.pp, the outer case is for the xcode
> version, with the inner case for the OS X version.  Here in
> xcode_cmdline_tools.pp it's the opposite.  That should probably get cleaned
> up.

I thought I had a good reason for this when I was writing it...but I can't remember now. Will do.

Comment 16

[[e-mail] (:bhearsum)]

Attachment: tested + cleaned up patch to upgrade xcode tools only for servo

This seems to do what we want. From a servo machine:
[[e-mail] ~]# puppet agent --test --environment bhearsum --noop
Info: Applying configuration version '00cdaec9b503'
Notice: /Stage[main]/Packages::Xcode_cmdline_tools/Packages::Pkgdmg[xcode462_cltools_10_76938260a]/Package[xcode462_cltools_10_76938260a.dmg]/ensure: current_value absent, should be present (noop)
Notice: Packages::Pkgdmg[xcode462_cltools_10_76938260a]: Would have triggered 'refresh' from 1 events
Notice: /Stage[main]/Jacuzzi_metadata/Exec[get_jacuzzi_metadata]/returns: current_value notrun, should be 0 (noop)
Notice: Class[Jacuzzi_metadata]: Would have triggered 'refresh' from 1 events
Notice: Class[Packages::Xcode_cmdline_tools]: Would have triggered 'refresh' from 1 events
Notice: Stage[main]: Would have triggered 'refresh' from 2 events
Notice: Finished catalog run in 48.06 seconds

From a bld-lion-r5:
[[e-mail] ~]#  puppet agent --test --environment bhearsum --server releng-puppet2.srv.releng.scl3.mozilla.com --noop
Info: Applying configuration version '00cdaec9b503'
Notice: /Stage[main]/Puppet::Atboot/File[/etc/puppet/puppetmasters.txt]/content: 
--- /etc/puppet/puppetmasters.txt	2014-05-22 08:44:49.000000000 -0700
+++ /var/folders/zz/zyxvpxvq6csfxvn_n0000000000000/T/puppet-file20140522-51256-nezkf2-0	2014-05-22 09:25:54.000000000 -0700
@@ -1,7 +1,7 @@
 releng-puppet1.srv.releng.scl3.mozilla.com
 releng-puppet2.srv.releng.scl3.mozilla.com
+releng-puppet2.srv.releng.use1.mozilla.com
+releng-puppet1.srv.releng.use1.mozilla.com
 releng-puppet1.srv.releng.usw2.mozilla.com
 releng-puppet2.build.scl1.mozilla.com
 releng-puppet2.srv.releng.usw2.mozilla.com
-releng-puppet1.srv.releng.use1.mozilla.com
-releng-puppet2.srv.releng.use1.mozilla.com

Notice: /Stage[main]/Puppet::Atboot/File[/etc/puppet/puppetmasters.txt]/content: current_value {md5}f65d2980e620b24301279ff633205d43, should be {md5}fa57ba81b3837c15bc1eb8d24ecd8365 (noop)
Notice: Class[Puppet::Atboot]: Would have triggered 'refresh' from 1 events
Notice: /Stage[main]/Jacuzzi_metadata/Exec[get_jacuzzi_metadata]/returns: current_value notrun, should be 0 (noop)
Notice: Class[Jacuzzi_metadata]: Would have triggered 'refresh' from 1 events
Notice: Stage[main]: Would have triggered 'refresh' from 2 events
Notice: Finished catalog run in 59.10 seconds

From a talos-r4-snow:
[[e-mail] ~]#  puppet agent --test --environment bhearsum --server releng-puppet2.srv.releng.scl3.mozilla.com --noop
Info: Applying configuration version '00cdaec9b503'
Notice: /Stage[main]/Tweaks::Cleanup/Exec[find /tmp/* -mmin +15 -print | xargs -n1 rm -rf]/returns: current_value notrun, should be 0 (noop)
Notice: Class[Tweaks::Cleanup]: Would have triggered 'refresh' from 1 events
Notice: /Stage[main]/Puppet::Atboot/File[/etc/puppet/puppetmasters.txt]/content: 
--- /etc/puppet/puppetmasters.txt	2014-05-20 11:14:26.000000000 -0700
+++ /tmp/puppet-file20140522-985-zoz2ht-0	2014-05-22 09:30:18.000000000 -0700
@@ -1,7 +1,7 @@
 releng-puppet2.build.scl1.mozilla.com
-releng-puppet2.srv.releng.scl3.mozilla.com
-releng-puppet1.srv.releng.scl3.mozilla.com
-releng-puppet2.srv.releng.use1.mozilla.com
-releng-puppet1.srv.releng.use1.mozilla.com
 releng-puppet2.srv.releng.usw2.mozilla.com
 releng-puppet1.srv.releng.usw2.mozilla.com
+releng-puppet2.srv.releng.use1.mozilla.com
+releng-puppet1.srv.releng.use1.mozilla.com
+releng-puppet2.srv.releng.scl3.mozilla.com
+releng-puppet1.srv.releng.scl3.mozilla.com

Notice: /Stage[main]/Puppet::Atboot/File[/etc/puppet/puppetmasters.txt]/content: current_value {md5}59b8d83ab9e3a591b20be29ad84becab, should be {md5}40be9e2cecb1a6159a807ba221bb6fe1 (noop)
Notice: Class[Puppet::Atboot]: Would have triggered 'refresh' from 1 events
Notice: Stage[main]: Would have triggered 'refresh' from 2 events
Notice: Finished catalog run in 56.03 seconds


From a talos-r5-mtnlion:
[[e-mail] ~]#  puppet agent --test --environment bhearsum --server releng-puppet2.srv.releng.scl3.mozilla.com --noop
Info: Applying configuration version '00cdaec9b503'
Notice: /Stage[main]/Tweaks::Cleanup/Exec[find /tmp/* -mmin +15 -print | xargs -n1 rm -rf]/returns: current_value notrun, should be 0 (noop)
Notice: Class[Tweaks::Cleanup]: Would have triggered 'refresh' from 1 events
Notice: /Stage[main]/Puppet::Atboot/File[/etc/puppet/puppetmasters.txt]/content: 
--- /etc/puppet/puppetmasters.txt	2014-05-22 08:58:24.000000000 -0700
+++ /var/folders/zz/zyxvpxvq6csfxvn_n0000000000000/T/puppet-file20140522-1294-1wrbbmm-0	2014-05-22 09:31:05.000000000 -0700
@@ -1,7 +1,7 @@
 releng-puppet2.srv.releng.scl3.mozilla.com
 releng-puppet1.srv.releng.scl3.mozilla.com
-releng-puppet1.srv.releng.usw2.mozilla.com
 releng-puppet2.build.scl1.mozilla.com
-releng-puppet2.srv.releng.usw2.mozilla.com
 releng-puppet1.srv.releng.use1.mozilla.com
 releng-puppet2.srv.releng.use1.mozilla.com
+releng-puppet2.srv.releng.usw2.mozilla.com
+releng-puppet1.srv.releng.usw2.mozilla.com

Notice: /Stage[main]/Puppet::Atboot/File[/etc/puppet/puppetmasters.txt]/content: current_value {md5}dfb48394bd05fc2270580bc34dbc81cb, should be {md5}3381988faa7bf54121cddbd8ebd347d6 (noop)
Notice: Class[Puppet::Atboot]: Would have triggered 'refresh' from 1 events
Notice: Stage[main]: Would have triggered 'refresh' from 2 events
Notice: Finished catalog run in 36.81 seconds

Comment 17

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8427102 [details] [diff] [review]
tested + cleaned up patch to upgrade xcode tools only for servo

Review of attachment 8427102 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I missed these bits in the feedback.

I think what you're intending is that 'include packages::xcode' actually does nothing if the version of xcode isn't known or specified, and similarly for xcode_cmdline_tools.  But that contradicts the principle of least surprise, since it means that some statements don't do what they say they do.

::: modules/config/manifests/base.pp
@@ +271,5 @@
>  
>      # slaveapi instance that slaverebooter should talk to.
>      $slaverebooter_slaveapi = ""
> +
> +    $xcode_version = undef

These will need some comments about the structure and where to look for options..

::: modules/packages/manifests/xcode.pp
@@ +51,2 @@
>              }
>          }

this will need a default indicating that the XCode version is invalid.

::: modules/packages/manifests/xcode_cmdline_tools.pp
@@ +64,1 @@
>                  }

this will need a default indicating that the XCode version is invalid.

::: modules/toplevel/manifests/slave/build/standard.pp
@@ +6,5 @@
>      include users::builder
>  
>      if ($::operatingsystem == "Darwin") {
>          include packages::xcode
> +        include packages::xcode_cmdline_tools

Is it even *possible* to install xcode and xcode_cmdline_tools on the same host?

Comment 18

[[e-mail] (:bhearsum)]

(In reply to Dustin J. Mitchell [:dustin] from comment #17)
> ::: modules/packages/manifests/xcode.pp
> @@ +51,2 @@
> >              }
> >          }
> 
> this will need a default indicating that the XCode version is invalid.
> 
> ::: modules/packages/manifests/xcode_cmdline_tools.pp
> @@ +64,1 @@
> >                  }
> 
> this will need a default indicating that the XCode version is invalid.

I removed these because the version handling is moved to the configs, with undef meaning "don't install anything".

> ::: modules/toplevel/manifests/slave/build/standard.pp
> @@ +6,5 @@
> >      include users::builder
> >  
> >      if ($::operatingsystem == "Darwin") {
> >          include packages::xcode
> > +        include packages::xcode_cmdline_tools
> 
> Is it even *possible* to install xcode and xcode_cmdline_tools on the same
> host?

Do you have an alternative suggestion, short of merging the two together? The fact that the releng and servo build machines are splintering suggests that maybe they need different toplevel classes. Maybe I should do that instead?

Comment 19

[Dustin J. Mitchell [:dustin] (he/him)]

Not other than merging the two -- and I don't see that as being particularly difficult.

They do have in common that an OS X build machine needs xcode of some sort.  What's still causing issues is that servo needs a cmdline_tools version of xcode, while moco needs a full version of xcode.  But if we fold that distinction into the version number, then there's no issue, right?

Comment 20

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug959005.patch

Untested, but this is the general idea I'm going for

Comment 21

[[e-mail] (:bhearsum)]

Attachment: dustin's way, tested

I fixed a syntax error and removed some duplication in servo-config.pp. Seems functionally equivalent to my most recent patch in my testing.

Comment 22

[[e-mail] (:bhearsum)]

Comment on attachment 8427198 [details] [diff] [review]
dustin's way, tested

There's a minor chance that this could bust a bunch of stuff so I'm going to wait until tomorrow morning to land it.

Comment 23

[[e-mail] (:bhearsum)]

Okay, both machines have Xcode command line tools 4.6.2 now. Someone is going to throw something at Bors to verify, but this should be all fixed now...

Thanks for your help, Dustin.