Add support for ARM32/ARM64 to our build infrastructure

[larsbergstrom]

r? @aneeshusa

I tested these changes inside of the awesome vagrant setup, and they seem to work locally, so I think that the next step is to review my miserable salt work.

There's some pretty hack-y stuff - assumptions of target triples differ between the Rust compiler, installed gcc binaries, and configure scripts, so there is some PATH and symlink hackery going on that could hopefully be reduced over time.

I've also renamed servo-linux-androidN to servo-linux-crossN, to better reflect the change. I'm assuming that we will need two cross builders online for {android, gonk, arm32, arm64} builds. Possibly three, as arm32/arm64 release builds are slow.

CC @edunham @Manishearth @metajack @mmatyas

[aneeshusa]

I don't have an opinion on renaming the android builders, but you should be aware that Salt matches minion keys by ID, so if you rename the minion you also need to do some key shuffling. The ideal way to do this is spin up a completely new machine with the servo-linux-crossN setup (i.e. it will generate a new key) and then decom the old servo-linux-android machine (and also unaccept/delete its key); this gives zero downtime and is more robust.

You can also just rename the machine (i.e., by setting the id key in the /etc/salt/minon config file on the machine) and restart the minion, but when it comes back up it won't be able to connect. To fix this, you'll also need to log into servo-master, cd into the /etc/salt/pki/master/minions/ directory, and rename the servo-linux-android1 file to servo-linux-cross1. This updates the Salt key database and the minion should automatically reconnect.

[aneeshusa]

It looks like the arm32 and arm64 setups are very similar, so this should use Jinja templating (probably some kind of loop like https://github.com/servo/saltfs/blob/master/common/init.sls#L40) in order to generate both configs.

[aneeshusa]

style nit: I tend to prefer dashes (-) to underscores (_) in state IDs.

[aneeshusa]

This should probably go into the style guide.

[aneeshusa]

I'd still like this to get added to the style guide, but not a blocker.

[aneeshusa]

These commands should use the creates parameter to avoid being re-executed on subsequent runs. Docs are here: https://docs.saltstack.com/en/2015.5/ref/states/all/salt.states.cmd.html#salt.states.cmd.wait.

[aneeshusa]

Actually, we should be use file.symlink inside a jinja loop for this logic instead. It's easier to see the intention and ensures we know exactly which symlinks are being created (and we get the creates behavior for free).

[aneeshusa]

Also, these states should require the /home/servo/bin state. They also shouldn't need to require the arm-dependencies state at all (you can symlink to files that don't exist yet), although this should be double-checked.

[larsbergstrom]

Do you have an example of such a jinja loop? The complication here is that there's also a file rename going on with the symlink (to change -linux to -unknown-linux). Sorry I'm such a noob at it, but I couldn't find a good example in the salt docs.

[larsbergstrom]

Hrm, your earlier template document on jinja is better; I'll look at that in the morning.

[aneeshusa]

Is the TRIPLET variable required at run time or is it only used to set CC and CXX? If the latter, I'd prefer to use python string formatting to bake the right value into those variables and avoid polluting the environment with the TRIPLET variable.

[larsbergstrom]

It's not - will remove!

[aneeshusa]

I think we should add some subdirectories inside this folder to keep the arm32 and arm64 binaries cleanly separated.

[larsbergstrom]

Will do!

[larsbergstrom]

I'm happy to move the servo-linux-android machines to servo-linux-cross* and shut down the old ones. We (@edunham and I) have been planning to move that machine, which is at linode, over to EC2, and this seems like a great opportunity to do so.

[larsbergstrom]

@aneeshusa OK, I think that I have finally managed to address everything except for trying to template the for loop over the symlinks, which I wasn't easily able to figure out. Does this look closer?

[larsbergstrom]

Failed in travis. Probably:

�[0;31m----------�[0m
    �[0;31m      ID: /usr/include/arm-linux-gnueabihf�[0m
    �[0;31mFunction: file.symlink�[0m
    �[0;31m  Result: False�[0m
    �[0;31m Comment: The following requisites were not found:
                                 require:
                                     archive: libs-arm-linux-gnueab�[0m
    �[0;31m Started: �[0m
    �[0;31mDuration: �[0m
�[0;31m     Changes:   �[0m
�[0;31m----------�[0m
    �[0;31m      ID: /usr/include/aarch64-linux-gnu�[0m
    �[0;31mFunction: file.symlink�[0m
    �[0;31m  Result: False�[0m
    �[0;31m Comment: The following requisites were not found:
                                 require:
                                     archive: libs-arm-linux-gnueab�[0m
    �[0;31m Started: �[0m
    �[0;31mDuration: �[0m
�[0;31m     Changes:   �[0m

[larsbergstrom]

Two more:

�[0;31m----------�[0m
    �[0;31m      ID: /home/servo/buildbot/slave�[0m
    �[0;31mFunction: file.recurse�[0m
    �[0;31m  Result: False�[0m
    �[0;31m Comment: #### /home/servo/buildbot/slave/buildbot.tac ####
              Unable to manage file: Jinja variable 'collections.OrderedDict object' has no attribute 'master'�[0m
    �[0;31m Started: 15:15:02.288078�[0m
    �[0;31mDuration: 6088.391 ms�[0m
�[0;31m----------�[0m
    �[0;31m      ID: buildbot-slave�[0m
    �[0;31mFunction: service.running�[0m
    �[0;31m  Result: False�[0m
    �[0;31m Comment: One or more requisite failed: buildbot.slave./home/servo/buildbot/slave�[0m
    �[0;31m Started: �[0m
    �[0;31mDuration: �[0m

[larsbergstrom]

@aneeshusa Can you please take a look at this? I'd love to get ARM32+ARM64 automation online, if this is closer to what you're thinking.

It's now passing Travis and seems to work in my local vagrant box.

I do still need some help with the more wizardly jinja rules for symlinks in place of my wonky cmd.run with the shell replacement scripts, but hopefully we could land without and fix that in a follow-up, if that works for you :-)

[aneeshusa]

These URLs need to have versions in them, so that when they're updated, the deployment workflow doesn't break due to hash mismatches. (Same for the arm64 file.)

[aneeshusa]

We should loop over the /usr/include, /usr/lib/ and /lib states with Jinja.

[aneeshusa]

These directories should also have versions in their path (like the Android NDK), so that Salt doesn't simply skip this state when we update the rootfs because it sees there is a previous version already extracted.

[aneeshusa]

For the symlinks:
If you look inside the /home/servo/bin/{aarch64-linux-gnu,arm-linux-gnueabihf} folders, you'll see 29 symlinks being created inside each folder. The symlinks have the form $PREFIX-$BINARY, where $PREFIX is aarch64-unknown-linux-gnu and arm-unknown-linux-gnueabihf, respectively, and $BINARY is things like as, gcc, and ld. Inside the Jinja block where you set targets, you can add the respective $PREFIX for each target to the set of variables, and you can use another Jinja set command to make a list of just the binaries we need (hopefully not all 29! we should be able to skip the versioned links at least, like gcc-4.8). Then, you can loop over the contents of this new Jinja variable of the binaries to generate all the symlink states.

Less important but nice to have:
Also, the symlink states should use - makedirs: True, and we should move the state that currently make the subfolders inside /home/servo/bin after the symlink states. That state should be amended to require all of the symlink states (using another Jinja loop to generate the requires), and we should add - clean: True to it.

Let me know if something is unclear here! IMO, the original bash script will be harder to read because of the substitutions inside the f2 variable - most people aren't very familiar with these. For that reason, I would like to get this fixed before merging if possible, but doing it in a follow-up wouldn't be out of the question.

[aneeshusa]

archive.extracted actually downloads the archive to a temporary location and extracts it directly to the location indicated by name, so it does not leave the tarball in this location. Please remove this line to prevent spurious re-downloads (without if_missing, it will check for the name argument instead, which is what we want.)

[larsbergstrom]

hrm...

�[0;31m----------�[0m
    �[0;31m      ID: /usr/includeaarch64-linux-gnu�[0m
    �[0;31mFunction: file.symlink�[0m
    �[0;31m  Result: False�[0m
    �[0;31m Comment: The following requisites were not found:
                                 require:
                                     archive: libs-/usr/include-aarch64-linux-gnu�[0m
    �[0;31m Started: �[0m
    �[0;31mDuration: �[0m
�[0;31m     Changes:   �[0m
�[0;31m----------�[0m
    �[0;31m      ID: /usr/libaarch64-linux-gnu�[0m
    �[0;31mFunction: file.symlink�[0m
    �[0;31m  Result: False�[0m
    �[0;31m Comment: The following requisites were not found:
                                 require:
                                     archive: libs-/usr/lib-aarch64-linux-gnu�[0m
    �[0;31m Started: �[0m
    �[0;31mDuration: �[0m
�[0;31m     Changes:   �[0m
�[0;31m----------�[0m
    �[0;31m      ID: /libaarch64-linux-gnu�[0m
    �[0;31mFunction: file.symlink�[0m
    �[0;31m  Result: False�[0m
    �[0;31m Comment: The following requisites were not found:
                                 require:
                                     archive: libs-/lib-aarch64-linux-gnu�[0m
    �[0;31m Started: �[0m
    �[0;31mDuration: �[0m
�[0;31m     Changes:   �[0m
�[0;36m

[aneeshusa]

Please pull the version (i.e. the 1) out into a variable (or two probably - we should be able to handle different versions for the rootfs files). I think a good place might be inside the targets variable.

[aneeshusa]

Use the (to-be-added) version and local_name variables to create these URLs where it's needed in the archive.extracted state, instead of setting them here.

[aneeshusa]

I think you may be missing a slash between these two.

[aneeshusa]

Same here, I think we need a slash between root and target.name.

[larsbergstrom]

OK, I think I've made all of the requested changes and travis passes :-)

[aneeshusa]

Looks like the file variables are redundant here with local_name. I think local_name is clearer, so let's keep those ones.

[aneeshusa]

style nit: this state is pretty short (6 lines), so I'd remove the empty newlines above and below it so the for loop lines are right next to the state. I find this makes it easier to see exactly where a short loop starts and ends, especially since we have nested loops.

[aneeshusa]

unknown_name sounds weird to me - maybe symlink_name instead?

[larsbergstrom]

OK, hopefully the last time! Thanks for sticking with this review through all of this :-)

[aneeshusa]

style nit: When I have for loops in require statements, I tend to keep the for loop at the same level of indentation so it doesn't break the block shape. Here, I would indent this line and the corresponding endfor over by six spaces.

[aneeshusa]

style nit: swap this endfor and the empty line above it, similar reasoning as above

[aneeshusa]

Getting close! Let me take a look inside Vagrant with the new changes.

Also, some of the style nits I'm making should probably get added to the style guide.

[aneeshusa]

whitespace: I'm seeing an extra space at the end of this line.

[aneeshusa]

How much RAM are you using in the VM? The Vagrantfile should give them each 1 GB, and free -m shows that I'm using about 850 MB right now. It's close for me, but there should be some breathing room. If it isn't enough, I'll put in a PR to bump it up (probably just for the cross builder) - just let me know how much is needed.

[aneeshusa]

Er, my numbers are only for running the highstate, I have no idea how much RAM it takes to actually build Servo, so bump it up in the Vagrantfile and let me know how much we need.

[aneeshusa]

Actually, we should replace the state that makes the now-empty subdirectories with a file.directory state that works on the entire /home/servo/bin folder, still with the - clean: True argument.

[larsbergstrom]

I'm using 1GB, but we need between 3--4GB to build Servo (I'm not sure exactly - cross builds seem to need more). I can do that separately. This is enough to clone Servo and do enough of the build to know that everything is working :-)

[larsbergstrom]

OK, everything is addressed, it runs and creates a builder that works for cross-compiles (there was one path thing that needed reflection into the buildbot config for the env). @aneeshusa r+?

[aneeshusa]

OK, this one is my fault, but we have to fix this: right now this state will blow away the archives we just extracted, due to saltstack/salt#26605, making the whole thing useless. To fix this, we should add a file.directory state that ensures the directory created by archive.extracted exists (so basically a no-op), then require that state instead of the libs- state in this state that has the - clean: True. (The bug is that the clean parameter only recognizes other file states.)

[aneeshusa]

It would be nice to put this state after the libs- state that it requires.

[aneeshusa]

We're getting some broken symlinks: gcov-tool for arm32 and arm64, as well as dwp and ld.gold on arm64. If we don't need these, we should remove them from the list.

[larsbergstrom]

OK, addressed the symlinks and the erroneous clean that needed an extra step.

[aneeshusa]

So so very close, this should be the last thing: we need to swap root and version to make these symlinks correct.

[aneeshusa]

Once we fix this last bug, I think this should be ready to go after a squash.

Before you deploy this, I saw we have an Android bug that I'd like to fix before you bring up the new -cross builder.

[aneeshusa]

Please squash so I can approve this.

[larsbergstrom]

Great, squashed!

[aneeshusa]

@bors-servo r+ (Also, is there a way to tell homu "I approve after a squash"?)

[larsbergstrom]

@aneeshusa There isn't, but generally what we do is say "r=me after squash" to people with review priviledges or "@bors-servo delegate" to people who do not, and then they can "@bors-servo r=aneeshusa" when it's ready.

[bors-servo]

📌 Commit 1c7b070 has been approved by aneeshusa"

[bors-servo]

⌛ Testing commit 1c7b070 with merge 0485aeb...

[larsbergstrom]

And speaking of, given that homu didn't notice it, I think that I need to open a new PR :-)

[bors-servo]

☀️ Test successful - travis

[larsbergstrom]

@aneeshusa What is the Android bug that you'd like me to fix before we bring up the new -cross builder?

I think that @edunham and I will probably be looking into that next week, as part of the Linode=>EC2 migration (the easy bit ).

[aneeshusa]

There's actually a bunch of intertwined Android fixes that are a bit involved (a lot of overlapping changes similar to ones we did here), so expect to see a PR from me in the next few days.

Let me know if you need any help with the migration.