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

Update android-18 menu entry to 8 for SDK r24 #290

Merged

Conversation

@aneeshusa
Copy link
Member

aneeshusa commented Apr 2, 2016

We're ignoring Travis builds for the cross builder and this fix needs to be verified in Vagrant (this VM takes a while to provision unfortunately). We may want to actually try running a build in Vagrant to see if this works.

We updated to Android SDK r24 in November from r23, but did not update
the menu entry to install the android-18 target. The new number comes
from the output of ./android sdk list.

We need to delete the /home/servo/android/sdk/r24.4.1 folder before applying this fix to the builders. Ideally this would get checked for by Salt, but currently you can only specify one file for the - creates: key of cmd.run, and our state creates the platform-tools dir and the android-18 dir. I'll open a Salt issue for this.

cc @larsbergstrom, servo/servo#10339


This change is Reviewable

@aneeshusa
Copy link
Member Author

aneeshusa commented Apr 2, 2016

Hmm, this seems to have installed android-20 in my Vagrant machine. :(

Newest version seems to install android-18 properly.

It seems that the output of `android list sdk` changes over time,
so we should not rely on its ordering of menu items. Instead, we can
use stable identifiers that don't change.
@aneeshusa aneeshusa force-pushed the aneeshusa:update-android-18-menu-entry-for-sdk-r24 branch from 6c7f577 to da097a4 Apr 2, 2016
@aneeshusa
Copy link
Member Author

aneeshusa commented Apr 2, 2016

Update: newest version is much simpler and seems to work properly in Vagrant. Can't believe I never tried this before.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Apr 2, 2016

Oh, nice! Is this ready to go?

@aneeshusa
Copy link
Member Author

aneeshusa commented Apr 2, 2016

Yup, this is ready to get reviewed - make sure to double check that it installs android-18 properly in Vagrant.

@aneeshusa
Copy link
Member Author

aneeshusa commented May 13, 2016

Ping on this; android-nightly was failing to compile previously (I think due to an LLVM bug), but a recent rustup means compilation now succeeds and this is again a blocker for sucessful android-nightly runs.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented May 24, 2016

@bors-servo
Copy link
Contributor

bors-servo commented May 24, 2016

📌 Commit da097a4 has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented May 24, 2016

Testing commit da097a4 with merge f1d5000...

bors-servo added a commit that referenced this pull request May 24, 2016
…24, r=larsbergstrom

Update android-18 menu entry to 8 for SDK r24

We're ignoring Travis builds for the cross builder and this fix needs to be verified in Vagrant (this VM takes a while to provision unfortunately). We may want to actually try running a build in Vagrant to see if this works.

We updated to Android SDK r24 in November from r23, but did not update
the menu entry to install the android-18 target. The new number comes
from the output of `./android sdk list`.

We need to delete the `/home/servo/android/sdk/r24.4.1` folder before applying this fix to the builders. Ideally this would get checked for by Salt, but currently you can only specify one file for the ` - creates: ` key of `cmd.run`, and our state creates the `platform-tools` dir and the `android-18` dir. I'll open a Salt issue for this.

cc @larsbergstrom, servo/servo#10339

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/290)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 24, 2016

☀️ Test successful - travis

@bors-servo bors-servo merged commit da097a4 into servo:master May 24, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@aneeshusa
Copy link
Member Author

aneeshusa commented May 24, 2016

Heads up - I believe this will require some manual intervention to get Salt to apply the changes. Deleting the /home/servo/android/sdk/r24.4.1 path before running a highstate should do the trick.
(The issue is that we only check for the platform tools being installed, not the android-18 SDK, with the creates parameter.)
I'll file a follow-up PR to make this happen automatically.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented May 24, 2016

Thanks! Indeed, I rolled this out manually after landing it and noticing that highstate thought that nothing needed to be modified :-)

@aneeshusa
Copy link
Member Author

aneeshusa commented May 25, 2016

See #383 for the follow-up PR.

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

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