Update to a newer version of the Android NDK #529

larsbergstrom · 2016-10-29T01:34:19Z

Comments from @mmatyas welcome :-)

There are two parts to this change.

Update the NDK. Google has both changed the DL URL format and removed the excutable bit in favor of a normal zipfile.
We no longer use the android standalone toolchain stuff, as that is not compatible with the LLVM libc++ that SpiderMonkey now requires us to use on Android.

This change is

aneeshusa · 2016-10-29T01:37:05Z

It sounds like those two changes are unrelated, so please split this into two commits, and add those justifications to the commit messages please :)

aneeshusa · 2016-10-29T01:38:47Z

servo-build-dependencies/android.sls

@@ -92,22 +93,13 @@ android-ndk:
      - user: servo
  cmd.run:
      # Need to filter log output to avoid hitting log limits on Travis CI
-    - name: '{{ common.servo_home }}/android/ndk/{{ android.ndk.version }}/android-ndk-{{ android.ndk.version }}-linux-x86_64.bin | grep -v Extracting'
+    - name: 'unzip {{ common.servo_home }}/android/ndk/{{ android.ndk.version }}/android-ndk-{{ android.ndk.version }}-linux-x86_64.zip'


archive.extracted supports unzipping, so please use that instead of manually running unzip.

aneeshusa · 2016-10-29T01:40:30Z

servo-build-dependencies/android.sls

@@ -28,6 +28,7 @@ android-dependencies:
      - lib32z1
      - libstdc++6
      - libgl1-mesa-dev
+      - unzip


It looks like Salt sometimes uses the binary and sometimes Python's zipfile module, so let's keep this even with archive.extracted.

Do you mean to remove this dependency and the use of unzip and then use a salt rule instead for the extraction?

To be more clear, please use the Salt archive.extracted state (with archive_type: zip) instead of manually downloading and extracting for android-ndk. (android-sdk has an example of this in the same file, and the docs are here.) Also keep the unzip package dependency, as Salt sometimes uses it internally.

mmatyas · 2016-10-29T10:59:04Z

servo-build-dependencies/map.jinja

@@ -7,8 +7,8 @@
      'sha512': '96fb71d78a8c2833afeba6df617edcd6cc4e37ecd0c3bec38c39e78204ed3c2bd54b138a56086bf5ccd95e372e3c36e72c1550c13df8232ec19537da93049284'
    },
    'ndk': {
-      'version': 'r10e',
-      'sha512': '8948c7bd1621e32dce554d5cd1268ffda2e9c5e6b2dda5b8cf0266ea60aa2dd6fddf8d290683fc1ef0b69d66c898226c7f52cc567dbb14352b4191ac19dfb371'
+      'version': 'r12b',


Personally I'm using r12, does it work with r12b too?

r12b is what I'm using on my machines. I haven't tried it with very-latest, r13.

mmatyas · 2016-10-29T11:12:49Z

Another problem that might occur is that everything works fine if you only have Java 7 or older installed, but if you also have Java 8 or 9, then certain libs want to build using the latest version, causing version difference and parsing errors during mach package. I'm not sure what kind of system the bots run on, but in case you get such errors, thats's the cause.

larsbergstrom · 2016-10-29T11:14:03Z

Hrm, we just use whatever the default-jdk is on Ubuntu (https://github.com/servo/saltfs/blob/master/servo-build-dependencies/android.sls#L23 ). Perhaps I should finally update us to use the more modern Android build system tools once this is fully landed :-)

mmatyas · 2016-10-29T11:23:50Z

default-jdk points to openjdk-8-jre on my Ubuntu 16.04, but on older systems, it was Java 7, and I had to manually add-apt-repository ppa:openjdk-r/ppa and install openjdk-7-jdk to make it work. Even after setting it as the default Java, the build system (probably one of the dependencies) still calls J8 somehow. But as long as you have Java 7 only, it should work fine.

larsbergstrom · 2016-10-31T21:51:55Z

@aneeshusa OK, does that look better?

larsbergstrom · 2016-11-01T21:13:39Z

OK, passing CI now! Squashed. r? :-)

aneeshusa

Overall approach looks good, and thanks for teaching me about the new requisite format in 2016.3 :)

A few more things to do:

Please update the first commit message to mention "as that is not compatible with the LLVM libc++ that SpiderMonkey now requires us to use on Android." in the body.
Update the second commit message to make the first line shorter (ideally < 50 chars) - perhaps move the details about the download now being a zip to the commit body.
Update the Buildbot configuration so we no longer pass the ANDROID_TOOLCHAIN variable.

Notes for deployment:

I believe I was having trouble getting Salt to auto-clean old things, so when this is rolled out we need to manually delete the old toolchain-related files.

aneeshusa · 2016-11-02T16:37:22Z

servo-build-dependencies/android.sls

    - source_hash: sha512={{ android.ndk.sha512 }}
+    - archive_format: zip
    - user: servo
    - group: servo
    - mode: 744


Remove mode and dir_mode, which aren't accepted by archive.extracted.

aneeshusa · 2016-11-02T16:37:22Z

servo-build-dependencies/android.sls

    - source_hash: sha512={{ android.ndk.sha512 }}
+    - archive_format: zip
    - user: servo


Also pass archive_user (see the android-sdk state for why, feel free to copy the justification comment down here as well.)

aneeshusa · 2016-11-02T16:37:22Z

servo-build-dependencies/android.sls

-    - name: '{{ common.servo_home }}/android/ndk/{{ android.ndk.version }}/android-ndk-{{ android.ndk.version }}-linux-x86_64.bin | grep -v Extracting'
-    - runas: servo
-    - cwd: {{ common.servo_home }}/android/ndk/{{ android.ndk.version }}
-    - creates: {{ common.servo_home }}/android/ndk/{{ android.ndk.version }}/android-ndk-{{ android.ndk.version }}


I haven't checked in Vagrant, but I'm assuming the archive.extracted state will also create this in the FS; convert this to a if_missing arg for archive.extracted.

aneeshusa · 2016-11-02T16:37:22Z

servo-build-dependencies/android.sls

    - source_hash: sha512={{ android.ndk.sha512 }}
+    - archive_format: zip
    - user: servo
    - group: servo
    - mode: 744
    - dir_mode: 755
    - makedirs: True


Remove this as well, as archive.extracted always makes the necessary parent directories and so doesn't accept makedirs.

larsbergstrom · 2016-11-02T21:25:55Z

Also updated commit messages :-)

aneeshusa · 2016-11-04T15:45:02Z

OK, tested this in Vagrant and it looks OK.

r=me after a few more commit message nits:

Don't have the first commit message all on one line; use "Remove usage of ... for building Android" as the first line (title), then make the rest into the body.
Leave off the periods at the end of the commit title (first line)

Don't forget to clean out the old toolchain/ dirs on the builders, as well as the old r10e NDK.


        Remove usage of the special toolchain for Android

The separate NDK toolchain installs are no longer compatible with the new LLVM libc++ that is required in order to build the SpiderMonkey JS engine.


        Update to a newer version of the Android NDK

This new version also is now only available in zip format.

larsbergstrom · 2016-11-09T20:26:03Z

@bors-servo r=aneeshusa

larsbergstrom · 2016-11-09T20:46:37Z

@bors-servo r=aneeshusa

bors-servo · 2016-11-09T20:46:40Z

📌 Commit aa5612d has been approved by aneeshusa

bors-servo · 2016-11-09T20:46:42Z

⚡ Test exempted - status


        Auto merge of #529 - larsbergstrom:android_ndk_updates, r=aneeshusa

@aneeshusa

Update to a newer version of the Android NDK r? @aneeshusa Comments from @mmatyas welcome :-) There are two parts to this change. 1) Update the NDK. Google has both changed the DL URL format and removed the excutable bit in favor of a normal zipfile. 2) We no longer use the android standalone toolchain stuff, as that is not compatible with the LLVM libc++ that SpiderMonkey now requires us to use on Android.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/529)

larsbergstrom · 2016-11-09T20:57:09Z

This has been deployed to servo-linux-cross*

larsbergstrom · 2016-11-09T22:04:18Z

Hrm, it appears that salt did not preserve the execute bit on some of these files?

servo@ip-172-31-34-111:~/android/ndk$ ls -al ./r12b/android-ndk-r12b/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-gcc
-rw-r--r-- 1 servo servo 784904 Nov  9 20:51 ./r12b/android-ndk-r12b/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-gcc

I did a quick find . -name bin | xargs chmod -R u+x in the ndk directory to try to get Android builds going, but that seems curious.

aneeshusa · 2017-04-20T06:09:15Z

The permissions not being extracted properly should be fixed by #639. (I didn't forget about this! 😄)

highfive assigned aneeshusa Oct 29, 2016

highfive added the S-awaiting-review label Oct 29, 2016

aneeshusa reviewed Oct 29, 2016

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Oct 29, 2016

mmatyas reviewed Oct 29, 2016

View changes

larsbergstrom force-pushed the larsbergstrom:android_ndk_updates branch from 4b029b2 to 104092e Oct 31, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 31, 2016

larsbergstrom force-pushed the larsbergstrom:android_ndk_updates branch from 2b98c9a to edf426a Nov 1, 2016

aneeshusa reviewed Nov 2, 2016

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Nov 2, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 2, 2016

larsbergstrom force-pushed the larsbergstrom:android_ndk_updates branch 2 times, most recently from 5c887bf to 4d263cc Nov 2, 2016

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Nov 4, 2016

larsbergstrom force-pushed the larsbergstrom:android_ndk_updates branch from 4d263cc to bd88ecb Nov 9, 2016

highfive removed the S-needs-code-changes label Nov 9, 2016

highfive added the S-awaiting-review label Nov 9, 2016

larsbergstrom added 2 commits Oct 31, 2016

Remove usage of the special toolchain for Android

2d0ba1c

The separate NDK toolchain installs are no longer compatible with the new LLVM libc++ that is required in order to build the SpiderMonkey JS engine.

Update to a newer version of the Android NDK

Loading status checks…

aa5612d

This new version also is now only available in zip format.

larsbergstrom force-pushed the larsbergstrom:android_ndk_updates branch from bd88ecb to aa5612d Nov 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 9, 2016

bors-servo merged commit aa5612d into servo:master Nov 9, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test exempted
Details

highfive removed the S-awaiting-merge label Nov 9, 2016

aneeshusa mentioned this pull request Apr 20, 2017

Add test that NDK is extracted properly #641

Open

servo / saltfs

Update to a newer version of the Android NDK #529

Update to a newer version of the Android NDK #529

larsbergstrom commented Oct 29, 2016 •

edited

aneeshusa commented Oct 29, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mmatyas commented Oct 29, 2016

larsbergstrom commented Oct 29, 2016

mmatyas commented Oct 29, 2016

larsbergstrom commented Oct 31, 2016

larsbergstrom commented Nov 1, 2016

aneeshusa left a comment •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

larsbergstrom commented Nov 2, 2016

aneeshusa commented Nov 4, 2016

larsbergstrom commented Nov 9, 2016

larsbergstrom commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

larsbergstrom commented Nov 9, 2016

larsbergstrom commented Nov 9, 2016 •

edited

aneeshusa commented Apr 20, 2017

servo / saltfs

Join GitHub today

Update to a newer version of the Android NDK #529

Update to a newer version of the Android NDK #529

Conversation

larsbergstrom commented Oct 29, 2016 • edited

aneeshusa commented Oct 29, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mmatyas commented Oct 29, 2016

larsbergstrom commented Oct 29, 2016

mmatyas commented Oct 29, 2016

larsbergstrom commented Oct 31, 2016

larsbergstrom commented Nov 1, 2016

aneeshusa left a comment • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

larsbergstrom commented Nov 2, 2016

aneeshusa commented Nov 4, 2016

larsbergstrom commented Nov 9, 2016

larsbergstrom commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

larsbergstrom commented Nov 9, 2016

larsbergstrom commented Nov 9, 2016 • edited

aneeshusa commented Apr 20, 2017

larsbergstrom commented Oct 29, 2016 •

edited

aneeshusa left a comment •

edited

larsbergstrom commented Nov 9, 2016 •

edited