Skip to content

Added swiftpm and other libs to Android toolchain #11870

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.

Sign up for GitHub

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

Jump to bottom
Merged
milseman merged 1 commit into from
Oct 23, 2017
Merged

Added swiftpm and other libs to Android toolchain #11870

milseman merged 1 commit into from
Oct 23, 2017

Conversation

@amraboelela
Copy link
Contributor

@amraboelela amraboelela commented Sep 12, 2017

These changes will add swift package management tools to the android toolchain, among other libraries and tools

@amraboelela
Copy link
Contributor Author

amraboelela commented Sep 12, 2017

@milseman Please check this :)

@milseman milseman requested a review from Rostepher September 12, 2017 20:46
@milseman
Copy link
Member

milseman commented Sep 12, 2017

@Rostepher, could you review this or let me know who is a more appropriate reviewer for this?

@milseman
Copy link
Member

milseman commented Sep 12, 2017

@swift-ci please smoke test

1 similar comment
@milseman
Copy link
Member

milseman commented Sep 12, 2017

@swift-ci please smoke test

@amraboelela
Copy link
Contributor Author

amraboelela commented Sep 12, 2017

Question. When you say @ swift-ci please smoke test, are there humans behind this or is it automatic thing? :)

@milseman
Copy link
Member

milseman commented Sep 12, 2017

Automatically handled by a bot. There are sometimes issues, of course, which are inherent to the problem of CI.

@Rostepher
Copy link
Contributor

Rostepher commented Sep 12, 2017

If you don't see a build start almost immediately then the CI bot probably didn't get the message.

@amraboelela
Copy link
Contributor Author

amraboelela commented Sep 12, 2017

Does that mean I can also initiate the test by saying @ swift-ci please smoke test ?

@milseman
Copy link
Member

milseman commented Sep 12, 2017

@amraboelela It might be limited to those with commit access, to help manage potential load on the bots.

@amraboelela
Copy link
Contributor Author

amraboelela commented Sep 12, 2017

@swift-ci please smoke test

@amraboelela
Copy link
Contributor Author

amraboelela commented Sep 12, 2017

Just testing :) so I guess it didn't work from me.

@amraboelela amraboelela force-pushed the android-build-toolchain2 branch from d7ebf25 to 969645c Compare September 12, 2017 23:52
@amraboelela
Copy link
Contributor Author

amraboelela commented Sep 12, 2017

Oh, the tests actually got started after a while, so maybe anybody can start them.

@amraboelela
Copy link
Contributor Author

amraboelela commented Sep 13, 2017

@swift-ci please smoke test

@amraboelela
Copy link
Contributor Author

amraboelela commented Sep 13, 2017

It got stuck at Waiting for status to be reported

@milseman
Copy link
Member

milseman commented Sep 13, 2017

@swift-ci please smoke test

@CodaFi
Copy link
Contributor

CodaFi commented Sep 14, 2017 •
edited
Loading

Not that I know what I'm taking about, but is it reasonable to constrain the script to only build for armv7e? Shouldn't this be something that's figured out from a target triple instead of hard-coded?

@milseman
Copy link
Member

milseman commented Sep 14, 2017

@Rostepher could you review this?

@compnerd
Copy link
Member

compnerd commented Sep 19, 2017

@CodaFi android only supports the A class of CPUs AFAIK. The NDK only provides the v7a bits, so I think that bit is reasonable.

@Rostepher
Copy link
Contributor

Rostepher commented Sep 19, 2017

@milseman I'll give this a look over in the morning.

Rostepher
Rostepher reviewed Sep 21, 2017
View reviewed changes
Copy link
Contributor

@Rostepher Rostepher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any issues with this, except for what @CodaFi pointed out with hardcoding the ARM version. I've not had any experience with building for Android yet so take my input with a grain of salt. @gottesmm Do you have any experience with Android builds?

@gottesmm
Copy link
Contributor

gottesmm commented Sep 22, 2017

No. I think @modocache is the best AFAIK.

@amraboelela amraboelela force-pushed the android-build-toolchain2 branch from eec5656 to b88b88c Compare October 12, 2017 03:34
@amraboelela amraboelela mentioned this pull request Oct 13, 2017
Merged
@alblue
Copy link
Contributor

alblue commented Oct 13, 2017

@swift-ci please smoke test

@alblue
Copy link
Contributor

alblue commented Oct 13, 2017

@amraboelela note that the swift-ci bot is picky about who it listens to - committers have access to trigger builds but not others. When it says 'waiting for status' it's really waiting for someone else to kick it off on your behalf.

@milseman
Copy link
Member

milseman commented Oct 13, 2017

@compnerd or @modocache what do you think of this change?

modocache
modocache approved these changes Oct 14, 2017
View reviewed changes
Copy link
Contributor

@modocache modocache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks!

I believe "armeabi-v7a" is hardcoded in a few spots in the build scripts and testing utilities for Android, so it doesn't bother me that it also is here. Maybe someone could create a task to clean that up and take care of it in a forthcoming pull request?

utils/android/README.md Outdated
Copy link
Contributor

@modocache modocache Oct 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion: I think expanding on what "Android toolchain" means here would help people who have some interest, but don't quite understand what a "toolchain" is, to use the scripts and tools in here. Ideally, I think this README should answer the question: "why would I want to build an Android toolchain?"

@amraboelela amraboelela force-pushed the android-build-toolchain2 branch 3 times, most recently from 2551bb2 to 888bc52 Compare October 16, 2017 17:05
xwu
xwu reviewed Oct 17, 2017
View reviewed changes
utils/android/README.md Outdated
Copy link
Collaborator

@xwu xwu Oct 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please capitalize "Swift" and correct the typo in the word "application".
Insert "the" before "Swift standard library" and before "Android environment".

@amraboelela amraboelela force-pushed the android-build-toolchain2 branch 5 times, most recently from e38f79f to d9ef07a Compare October 17, 2017 16:08
@milseman
Copy link
Member

milseman commented Oct 17, 2017

@amraboelela, do you want to do those fixes in this PR or a followup? I know it's been open for quite a while, and I have no strong opinions so long as @modocache and @xwu are fine with either.

@amraboelela
Copy link
Contributor Author

amraboelela commented Oct 17, 2017

@milseman I already did the changes that @modocache and @xwu asked me to do.

@milseman
Copy link
Member

milseman commented Oct 17, 2017

@swift-ci please smoke test

@amraboelela amraboelela force-pushed the android-build-toolchain2 branch from ff6a306 to 0e9f909 Compare October 18, 2017 03:17
@alblue
Copy link
Contributor

alblue commented Oct 18, 2017

@swift-ci please smoke test

@milseman
Copy link
Member

milseman commented Oct 18, 2017

@swift-ci please smoke test Linux Platform

xwu
xwu reviewed Oct 19, 2017
View reviewed changes
utils/android/README.md Outdated
Copy link
Collaborator

@xwu xwu Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please capitalize all instances of "Swift" :)

@amraboelela amraboelela force-pushed the android-build-toolchain2 branch from 0e9f909 to eacb632 Compare October 19, 2017 14:29
@milseman
Copy link
Member

milseman commented Oct 19, 2017

@swift-ci please smoke test

@amraboelela amraboelela force-pushed the android-build-toolchain2 branch from a4173c1 to 3936714 Compare October 20, 2017 17:23
@amraboelela
Copy link
Contributor Author

amraboelela commented Oct 20, 2017

Did I ruin the smoke test by doing merge from master?

@Rostepher
Copy link
Contributor

Rostepher commented Oct 20, 2017

Any new commits will update the status of the PR and require a new smoke-test run. Same goes for force pushing a git rebase or a new merge commit.

@Rostepher
Copy link
Contributor

Rostepher commented Oct 20, 2017

@swift-ci please smoke test

@amraboelela amraboelela mentioned this pull request Oct 20, 2017
Merged
@amraboelela amraboelela force-pushed the android-build-toolchain2 branch from 30b2470 to 1d75bc8 Compare October 21, 2017 02:05
@amraboelela
Copy link
Contributor Author

amraboelela commented Oct 21, 2017

Did another merge from master because the Linux smoke test failed.

@modocache
Copy link
Contributor

modocache commented Oct 21, 2017

@swift-ci please smoke test

@milseman milseman merged commit edb078f into swiftlang:master Oct 23, 2017
@amraboelela amraboelela deleted the android-build-toolchain2 branch October 23, 2017 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xwu xwu xwu left review comments

@Rostepher Rostepher Rostepher left review comments

@modocache modocache modocache approved these changes

@compnerd compnerd Awaiting requested review from compnerd

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@amraboelela @milseman @Rostepher @CodaFi @compnerd @gottesmm @alblue @modocache @xwu