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

reference compiler-builtins by git source #322

Merged
merged 1 commit into from
Sep 1, 2021

Conversation

a-nickol
Copy link
Contributor

@a-nickol a-nickol commented Sep 1, 2021

Fixes #320, but I am not sure, what the previous behavior was!

@a-nickol a-nickol force-pushed the bugfix/compiler-buildtins_not_found branch from 3173418 to 967f020 Compare September 1, 2021 14:55
@RalfJung
Copy link
Collaborator

RalfJung commented Sep 1, 2021

Is it because of the missing source for compiler_builtins in rust-src? I am not sure if it had been there before.

I don't think it has been there before, rust-lang/rust#78209 shows it has been pulled in from crates.io for a long time already.

But then why doesn't it pick up the version from https://crates.io/crates/compiler_builtins? The git reference will likely not match the exact version that this std should be built with.

@RalfJung
Copy link
Collaborator

RalfJung commented Sep 1, 2021

So the question is, in that default case where xargo adds a compiler_builtins dependency itself, which version should it add? Can we just set the version to * to get the latest version from crates.io? That seems less risky than using the latest git version...

@a-nickol
Copy link
Contributor Author

a-nickol commented Sep 1, 2021

I will do some more research, but this should be the right path.

@RalfJung
Copy link
Collaborator

RalfJung commented Sep 1, 2021

To be specific, what do you mean by "this"? IMO using a crates.io version is preferable over using the git version.

@a-nickol
Copy link
Contributor Author

a-nickol commented Sep 1, 2021

I tested if the version is somehow related to the usage of not usage of the version declaration.

When using version = "*" in the Cargo.toml the version of compiler_builtins is set to 0.1.50.

Without the version and without the git repository the version is set to 0.1.49.

@a-nickol
Copy link
Contributor Author

a-nickol commented Sep 1, 2021

Should we read the version of the dependency by parsing the Cargo.lock of the toolchain, e.g .rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/Cargo.lock?

For my build it would be then: compiler_builtins = { version = "0.1.49" }.

Since it is already copied over to the build directory, we would just have to parse it.

@RalfJung
Copy link
Collaborator

RalfJung commented Sep 1, 2021

When using version = "*" in the Cargo.toml the version of compiler_builtins is set to 0.1.50.

Yeah, that is the latest version.

Without the version and without the git repository the version is set to 0.1.49.

Isn't there an error in this case?

When liballoc is being built, we pull in compiler_builtins via that, and the version is given by the Cargo.lock file shipped in rust-src. But I don't think we should get into the business of parsing the lockfile.

Honestly I am not sure what people expect when giving no Xargo.toml; this case is not really well-defined...

@a-nickol
Copy link
Contributor Author

a-nickol commented Sep 1, 2021

I made a mistake. I think using version = "*" just works perfect, since we are copying the Cargo.lock from std, the correct version from std is selected! I will change the commit.

@a-nickol a-nickol force-pushed the bugfix/compiler-buildtins_not_found branch from 6b4101c to 9a6369e Compare September 1, 2021 19:57
@a-nickol
Copy link
Contributor Author

a-nickol commented Sep 1, 2021

On my machine the build even passes with the nightly-2019-12-09-toolchain when using the proposed solution.

…compiler_builtins is selected by Cargo.lock from the rust-src.
@a-nickol a-nickol force-pushed the bugfix/compiler-buildtins_not_found branch from 9a6369e to 747dc36 Compare September 1, 2021 20:09
@RalfJung
Copy link
Collaborator

RalfJung commented Sep 1, 2021

This looks great, thanks :).

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 1, 2021

Build succeeded:

@bors bors bot merged commit 807afd0 into japaric:master Sep 1, 2021
@a-nickol a-nickol deleted the bugfix/compiler-buildtins_not_found branch September 2, 2021 04:07
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.

Cargo cannot build compiler_builtins for aarch64-unknown-linux-musl
2 participants