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

CI suddenly not working #183

Closed
marioortizmanero opened this issue Feb 12, 2021 · 5 comments · Fixed by #184
Closed

CI suddenly not working #183

marioortizmanero opened this issue Feb 12, 2021 · 5 comments · Fixed by #184
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@marioortizmanero
Copy link
Collaborator

Describe the bug

Seems like the CI has stopped passing suddenly for cross-compilation on all platforms for reqwest. This has started happening in #182. I'm unable to reproduce on my current platform, without cross-compilation, on a x86_64 GNU/Linux.

I suspect it's due to #175, since the linking errors are similar to when the Tokio upgrade failed, see #164.

I'll investigate and try to see what's causing it.

@marioortizmanero marioortizmanero added bug Something isn't working help wanted Extra attention is needed labels Feb 12, 2021
@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Feb 12, 2021

Can confirm that this doesn't happen when downgrading to reqwest 0.10.7

reqwest 0.11 upgraded hyper to v0.14, so it might be related to that.

@marioortizmanero
Copy link
Collaborator Author

I've opened an issue at hyperium/hyper#2434, since it seems to be related to hyper.

@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Feb 12, 2021

I've finally found out why it doesn't work: starting at some commit from hyper it won't compile unless you run export CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER=aarch64-linux-gnu-gcc. I found this in cross' Dockerfile for this architecture, see https://github.com/rust-embedded/cross/blob/master/docker/Dockerfile.aarch64-unknown-linux-gnu.

To avoid these kind of issues and complicated installations, shall we try to use cross instead of cargo, @ramsayleung? It includes dockerfiles with the required dependencies and such. That, or just stop cross-compilating. Not sure if we should be worring about these kind of issues in such a high level library.

@ramsayleung
Copy link
Owner

Great to know that you figure out the reason :)

That, or just stop cross-compilating. Not sure if we should be worring about these kind of issues in such a high level library.

About one year ago, Marcel provided new features to support other tls implementations besides openssl in order make it easier to cross compile when using this crate in the PR #76. So I think it would be better to support cross-compiling if possible, since it seems some developers use rspotify in some architectures like ARM.

To avoid these kind of issues and complicated installations, shall we try to use cross instead of cargo

Is it possible to fix this CI issue by exporting this environment variable in Github Action configuration?

export CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER=aarch64-linux-gnu-gcc

I personally prefer to solve this issue with costing as less as possible, with built-in functionality of cargo, instead of including extra required dependencies :)

@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Feb 12, 2021

About one year ago, Marcel provided new features to support other tls implementations besides openssl in order make it easier to cross compile when using this crate in the PR #76. So I think it would be better to support cross-compiling if possible, since it seems some developers use rspotify in some architectures like ARM.

Sure! Testing can never be too much :)

Is it possible to fix this CI issue by exporting this environment variable in Github Action configuration?

Yes, but the thing is that you need something similar for each architecture, and they might break in the future again. Meaning that we would have to maintain the cross-compilation setup manually. The dockerfiles from cross also have more env variables we might need in the future. It might "cost" less in terms of overhead, but the workflow file will definitely be smaller, as all that's needed is installing docker and then cross.

To be fair cross might be too much, as it even installs Qemu to run the tests. But we could take advantage of that and do test + build, instead of just build as we're doing right now, if you care about that. If we just keep using build we don't really need cross. Just wanted to consider it.

Otherwise we can just take a look at cross's dockerfiles and copy the interesting parts, which seems to be only an env variable we're missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants