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

Using --extern is apparently not equivalent to --sysroot #40

Closed
alexcrichton opened this issue Sep 6, 2019 · 13 comments · Fixed by rust-lang/cargo#7421
Closed

Using --extern is apparently not equivalent to --sysroot #40

alexcrichton opened this issue Sep 6, 2019 · 13 comments · Fixed by rust-lang/cargo#7421
Labels
implementation Implementation exploration and tracking issues

Comments

@alexcrichton
Copy link
Member

Testing out building a crate graph with -Z build-std=std,alloc (note that the need to pass alloc is a separate bug) ends up yielding:

   Compiling crossbeam-utils v0.6.6
error: macro-expanded `extern crate` items cannot shadow names passed with `--extern`
  --> /home/alex/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/crossbeam-utils-0.6.6/src/lib.rs:43:9
   |
43 |         extern crate std as alloc;
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

error: Could not compile `crossbeam-utils`.

To learn more, run the command again with --verbose.

Sure enough this code:

macro_rules! a {
    () => (extern crate std as alloc;)
}
a!();

fails to compile:

$ rustc +nightly foo.rs --crate-type rlib --extern alloc -Z unstable-options
error: macro-expanded `extern crate` items cannot shadow names passed with `--extern`
 --> foo.rs:2:12
  |
2 |     () => (extern crate std as alloc;)
  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^
3 | }
4 | a!();
  | ----- in this macro invocation

error: aborting due to previous error

I'm not entirely sure the origin of this error, but it seems that we either need to:

  • Instead use --sysroot
  • Figure out if rustc can avoid error'ing in this case (maybe via more cli flags)
@alexcrichton alexcrichton added the implementation Implementation exploration and tracking issues label Sep 6, 2019
@Ericson2314
Copy link

Hmm what happens if you pass an empty sysroot? (See #31 where it mentions that). Perhaps there is an issue of search priority?

@ehuss
Copy link
Contributor

ehuss commented Sep 6, 2019

Ouch. Looks like this stems from rust-lang/rust#54658 (comment).

@Ericson2314 redirecting --sysroot doesn't matter here.

I don't really understand why they are not allowed to shadow. Might be good to better understand that (and why sysroot is allowed to be shadowed but not --extern).

@ehuss
Copy link
Contributor

ehuss commented Sep 7, 2019

@petrochenkov can you explain why a macro-expanded extern crate is allowed to shadow a sysroot crate, but not one from --extern? Is this restriction intended to be permanent?

For context, this project is to add standard library support to Cargo. My original strategy was to use --extern flags to tell rustc where to load the newly built std crates. However, it runs afoul of the check added in rust-lang/rust#54658 for things like this idiom in crossbeam-utils.

@petrochenkov
Copy link

petrochenkov commented Sep 7, 2019

@ehuss
Sysroot crates do not exist from name resolution point of view, so they cannot be shadowed.
extern crate my_sysroot; is the only entity that pulls a crate from sysroot (or a crate from -L directories) into name resolution.
(Note, that std and core specifically behave as if --extern std/core is passed implicitly, so they exist from name resolution point of view, and can be affected by shadowing restrictions.)

The shadowing restriction can be relaxed (by using may_appear_after instead of expansion != ExpnId::root(), rust-lang/rust#54658 was a bit lazy in this regard), but not removed entirely.

@alexcrichton
Copy link
Member Author

Hm so now this is sort of an interesting question. I think that we want -Z build-std to have the same semantics both if you use and if you don't. It looks like both core and std are implicit names in name resolution today, but both proc_macro and alloc, the other two stable crates, need to be implicitly exported. By using --extern alloc=... and --extern proc_macro=... we're actually changing the semantics of -Z build-std code because they don't need to say extern crate alloc. That seems bad!

Now the "solution" to this is probably to use --sysroot, but I'm realizing now I don't actually think that this is an easily viable option. The reason for this is that crates have implicit access to all crates in the sysroot, including the dependencies of std itself. For example you can do extern crate backtrace; today on nightly but it just gives you a warning about the usage of the rustc_private unstable feature. If we don't build crates with -Zforce-unstable-if-unmarked then crates will have implicit stable access to backtrace, for example, using -Zbuild-std.

So I think that our main goal should be parity between "normal mode" and -Z build-std mode. We can't today achieve that with --extern both because of this bug and also because today stable Rust requires extern crate alloc, even though alloc is stable. We also can't achieve it today with --sysroot because we have no way of building the dependencies of libstd as unstable without a -Z flag.

I think the best way forward, in general, is to "basically do the same thing" as rust-lang/rust. I think that we should use --sysroot (or -L) to load libstd crates, and we should also figure out how to mark them as inaccessible (maybe that's with the -Z flag of today, maybe not).

@ehuss
Copy link
Contributor

ehuss commented Sep 9, 2019

Using --sysroot is doable but much more complicated, and will require a lot of changes. I'd like to make sure we're certain it is the way to go. I don't mind rewriting it, I just don't want to make this change haphazardly. (Using --sysroot was my initial approach, but I was lured by the simplicity of using --extern.)

I was originally thinking alloc and proc_macro would need to be explicitly mentioned as dependencies in Cargo.toml, otherwise the --extern flag wouldn't be used. However, I see that would be a backwards-incompatible change for dependencies that aren't std-aware.

Hm, it's a bit unfortunate, but I can't think of better options.

@alexcrichton
Copy link
Member Author

My current thinking is we should do one of two things:

  • Use --sysroot in Cargo, compile everything into the sysroot. We'd compile everything with -Zforce-unstable-if-unmarked.

  • Add a new flag to rustc, like --extern-sysroot NAME=PATH. That would behave "as if the crate was in the sysroot" so it's available for extern crate but it doesn't inject names into resolution as --extern does.

I'm a fan personally of avoiding rustc changes if we can, but you're probably not wrong in that this is a much easier rustc change than a Cargo change. I was sort of hoping we could "just change the --out-dir" for libstd crates to the sysroot and don't pass --extern at all for sysroot crates in Cargo, but I suspect that's a bit too naive.

@ehuss
Copy link
Contributor

ehuss commented Sep 23, 2019

@alexcrichton I've been blocked for a while trying to think of a way to get the mock test suite to work. The fundamental problem is that there are duplicate core or std crate errors because there is both the mock one and the real one in the link path. Do you have any ideas on how the mock suite can be changed to work?

@alexcrichton
Copy link
Member Author

Oh dear yeah I can see where that would cause problems. Want to gist what you've got and I can try to poke around?

It may be that "mock std" either fundamentally no longer works or we can add name = "something-other-than-std" annotations and hack our way to success. It may be sort of fundamentally a broken idea now though, which would be a bummer!

@ehuss
Copy link
Contributor

ehuss commented Sep 23, 2019

Here is a work-in-progress (no test changes): ehuss/cargo@1ed8760

I've been trying a variety of things but keep hitting different problems. I was thinking of manually adding --extern flags into the original sysroot, but I keep hitting various issues.

@alexcrichton
Copy link
Member Author

Not my proudest moment, but I think this does the trick - alexcrichton/cargo@dead4d0

bors added a commit to rust-lang/cargo that referenced this issue Sep 25, 2019
Change build-std to use --sysroot

This transitions build-std to use `--sysroot` instead of `--extern`. This is necessary because existing crates have a certain expectation of how standard library crates are exposed. It was intended that explicit dependencies in `Cargo.toml` would solve this problem, but I didn't really consider this would be a backwards-incompatible change, so using `--sysroot` is probably the best way to go, even though it's not ideal.

Closes rust-lang/wg-cargo-std-aware#31
Closes rust-lang/wg-cargo-std-aware#40
@alexcrichton
Copy link
Member Author

Fixed in rust-lang/cargo#7421

@Ericson2314
Copy link

--extern-sysroot is a much better fix, I believe, not just easier.

bors added a commit to rust-lang/cargo that referenced this issue Dec 12, 2019
Switch build-std to use --extern

Switch `build-std` to use `--extern` flags instead of `--sysroot`.

This is mostly a revert of #7421. It uses the new extern flag options introduced in rust-lang/rust#67074. It avoids modifying the extern prelude which was the source of the problem of rust-lang/wg-cargo-std-aware#40.

Closes rust-lang/wg-cargo-std-aware#49
Reopens rust-lang/wg-cargo-std-aware#31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implementation Implementation exploration and tracking issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants