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

Add support for all cross targets #72

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Susurrus
Copy link

This also consolidates platform targets so it's easier to see what's what (copies from cross' .travis.yml).

The one thing that really needs to be investigated here is which targets should have DISABLE_TESTS set. I figured dragonfly should because all the BSDs do, and I would think that the windows and bare-metal targets should as well, but I didn't set that initially.

@japaric
Copy link
Owner

japaric commented Apr 19, 2017

@Susurrus thanks for the PR.

I don't think it makes sense to add the thumb targets here. They are very different from the other "std" targets. Pretty much no binary crate will compile for the thumb targets and for the std targets so deploys don't make sense for them. I think it would be better to remove them or strongly note that they don't support std.

Could you look into the errors. I think there's one target which is not currently supported by cross (it needs to be removed) and some targets for which cross is not installing std (that needs to be fixed in cross)

@Susurrus
Copy link
Author

@japaric I've commented out the thumb targets with a clear explanation. This should prevent people (like I did) try to add them in future PRs because they saw cross supported them.

I've corrected those 2 platforms that didn't actually exist and I've also added Android targets that I didn't add the first time and not running tests, which I think is appropriate. This is still WIP as I need to wait and see how Travis shakes out with this revision.

@Susurrus
Copy link
Author

The two ARM/musl targets are both failing because of a bug in musl (see here).

Windows is failing with a really weird bug that may be a bug in glibc in msys.

And Mac is failing because it couldn't download necessary files from GitHub I think. Maybe need to through a retry into the code there.

And dragonfly is failing because it's unsupported, so I'm going to remove it.

@japaric
Copy link
Owner

japaric commented Apr 20, 2017

The two ARM/musl targets are both failing because of a bug in musl

Hmm, those targets were working before and I think we haven't change their docker images in a while. So maybe the problem originates in the libc or gcc crate? As in a new release of one of those crates broke these targets.

@Susurrus
Copy link
Author

It has to do with missing #includes in the C code, which should be exclusively a musl bug (see the link I included in my previous comment). I wonder if they released a new version since you were first testing this.

@Susurrus
Copy link
Author

Susurrus commented Jun 30, 2017

We want to support android and some of these other targets with nix, so I'm getting back to this now to hopefully get all these targets supported. I've removed the dragonfly target as rust doesn't release std for it, but the rest I'm going to retry and see if they've been patched as I think they were library issues.

@Susurrus
Copy link
Author

So I'm assuming the mac build that's stalled right now will finish successfully like the rest of them, so then it's just the musl builds that are failing. Should I just remove them and post issues about them in the issue tracker or leave them in but commented out with a FIXME?

@CryZe
Copy link

CryZe commented Jun 30, 2017

iOS could be added too. Here's all the ones I was able to get running with trust / cross: https://travis-ci.org/CryZe/livesplit-core/builds/248531278

@Susurrus
Copy link
Author

@CryZe Awesome, thanks for the heads up. Added those to the list as well.

@Susurrus
Copy link
Author

Also, it's interesting that you are able to get *-linux-musleabi* targets to succeed when they don't work in the simple test case used here.

@Susurrus
Copy link
Author

@CryZe Did you have any problems getting ios targets building correct? Currently all those builds are failing complaining about the std crate missing.

@CryZe
Copy link

CryZe commented Jun 30, 2017

Yeah I did, so I hacked this in: https://github.com/CryZe/livesplit-core/blob/master/ci/install.sh#L13-L29

There's probably a cleaner solution.

@Susurrus
Copy link
Author

@CryZe Do you think that's the right way to support those targets? Seems weird the other targets don't need similar code.

@CryZe
Copy link

CryZe commented Jun 30, 2017

I don't think that's the right way. I would try to figure out why cross' iOS targets are different. I assume the docker images just have core or something.

@Susurrus
Copy link
Author

I think iOS builds are just done on Mac directly, they aren't done on iOS or in a docker image. So these might end up needing to be special-cased because of that.

Adds Android, iOS, Linux/ARM, and Windows. Bare-metal
thumb targets are listed, because they are supported by cross, but they
aren't enabled by default as they don't support std and will likely not
but used by most projects.

I've left off the *linux-musleabi* targets as they are failing with an
odd error that is likely due to musl itself.
@Susurrus
Copy link
Author

Susurrus commented Jul 3, 2017

@japaric Care to weigh in here? This should be easy to merge assuming the technique @CryZe used for support iOS builds is fine.

@Susurrus
Copy link
Author

Susurrus commented Jul 3, 2017

@CryZe I noticed that you aren't testing on x86_64-android. It doesn't work for me in my testing, but I wanted to see if you even tried it before I start digging into it.

@Susurrus
Copy link
Author

Susurrus commented Jul 3, 2017

As cross says it's supportes.

@CryZe
Copy link

CryZe commented Jul 3, 2017

I can't find any reason as to why I have turned them off. So I guess I just assumed they wouldn't work.

@Susurrus
Copy link
Author

Susurrus commented Jul 3, 2017

Well I'm getting the following error. I'd be curious if you're getting the same (I'd expect you would).

cross build --target x86_64-linux-android
    Updating git repository `https://github.com/rust-lang/libc`
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading tempdir v0.3.5
 Downloading rand v0.3.15
 Downloading libc v0.2.24
 Downloading rustc_version v0.1.7
 Downloading winapi-build v0.1.1
 Downloading winapi v0.2.8
 Downloading tempfile v2.1.5
 Downloading lazy_static v0.2.8
 Downloading gcc v0.3.51
 Downloading kernel32-sys v0.2.2
 Downloading void v1.0.2
 Downloading semver v0.1.20
 Downloading bitflags v0.9.1
 Downloading cfg-if v0.1.1
Unable to find image 'japaric/x86_64-linux-android:v0.1.11' locally
v0.1.11: Pulling from japaric/x86_64-linux-android
Status: Downloaded newer image for japaric/x86_64-linux-android:v0.1.11
warning: unused manifest key: package.categories
error: failed to run `rustc` to learn about target-specific information
To learn more, run the command again with --verbose.

@Susurrus
Copy link
Author

@japaric Just wanted to ping you on this? Shouldn't require too much review to get this merged.

@japaric
Copy link
Owner

japaric commented Sep 14, 2017

@Susurrus sorry, this PR fell off my radar.

It LGTM in principle. I'm actually surprised that the ios targets seem to work. Could we add the rustup target ios logic to Cross? I'd rather not add target specific logic here; that's why Cross is for.

Has anyone tested the ios binaries? I'd rather not do deploys / binary releases of targets that are unknown to actually work ... I know that the android targets work fine in Termux, at least.

@Susurrus
Copy link
Author

Susurrus commented Dec 2, 2017

@japaric This was merged into cross in cross-rs/cross#89. I don't have an iOS system to test on, so I guess this is stalled until someone steps up for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants