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

Stabilize --extern flag without a path. #64882

Merged
merged 8 commits into from
Nov 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/doc/rustc/src/codegen-options/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,11 @@ in software.
## prefer-dynamic

By default, `rustc` prefers to statically link dependencies. This option will
make it use dynamic linking instead.
indicate that dynamic linking should be used if possible if both a static and
dynamic versions of a library are available. There is an internal algorithm
for determining whether or not it is possible to statically or dynamically
link with a dependency. For example, `cdylib` crate types may only use static
linkage.

## no-integrated-as

Expand Down
30 changes: 25 additions & 5 deletions src/doc/rustc/src/command-line-arguments.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ to `#[cfg(verbose)]` and `#[cfg(feature = "serde")]` respectively.
<a id="option-l-search-path"></a>
Copy link
Member

Choose a reason for hiding this comment

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

This line is making the html check fails because the id is not unique.

## `-L`: add a directory to the library search path

When looking for external crates or libraries, a directory passed to this flag
will be searched.
The `-L` flag adds a path to search for external crates and libraries.

The kind of search path can optionally be specified with the form `-L
KIND=PATH` where `KIND` may be one of:
Expand Down Expand Up @@ -262,9 +261,30 @@ This flag, when combined with other flags, makes them produce extra output.
<a id="option-extern"></a>
## `--extern`: specify where an external library is located

This flag allows you to pass the name and location of an external crate that
will be linked into the crate you are building. This flag may be specified
multiple times. The format of the value should be `CRATENAME=PATH`.
This flag allows you to pass the name and location for an external crate of a
direct dependency. Indirect dependencies (dependencies of dependencies) are
located using the [`-L` flag](#option-l-search-path). The given crate name is
added to the [extern prelude], which is the same as specifying `extern crate`
within the root module. The given crate name does not need to match the name
the library was built with.

This flag may be specified multiple times. This flag takes an argument with
either of the following formats:

* `CRATENAME=PATH` — Indicates the given crate is found at the given path.
* `CRATENAME` — Indicates the given crate may be found in the search path,
such as within the sysroot or via the `-L` flag.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have tests ensuring that sysroot crates, e.g. private rustc details emit an error when used on a stable compiler? (Also when an explicit path is used to them, not just a search.)

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if both --extern CRATENAME=PATH and --extern CRATENAME are specified for the same CRATENAME? It would be nice to document that.
AFAIR, multiple --extern CRATENAME=PATHs with the same CRATENAME are possible if they lead to different kinds of libraries, like rlib and dylib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are about 30 tests doing various things with pathless --extern (search for // compile-flags:.*--extern).

I have added some more specific tests:

  • run-make-fulldeps/extern-flag-pathless: Shows what happens when you mix pathless with path --extern flags. Also demonstrates the preference of rlib over dylib.
  • ui-fulldeps/pathless-extern-unstable.rs: Shows --extern rustc is not allowed.
  • ui/pathless-extern-ok.rs: Basic test for a sysroot crate.

I don't have a good intuition of how sophisticated rustc tests should be. They are also a bit disorganized, so I didn't know where to stick them, or which style would be preferred. I think the dylib tests should be safe from a cross platform standpoint (I believe they only run on Linux?).

I updated the documentation with some more detail. I never really know how detailed rustc docs should be. I intentionally left the prefer-dynamic algorithm vague. It is described somewhat in src/librustc_metadata/dependency_format.rs. I figure since it is complex, and possibly subject to change, it may not be worthwhile. I can add more or remove some if desired.


The same crate name may be specified multiple times for different crate types.
If both an `rlib` and `dylib` are found, an internal algorithm is used to
decide which to use for linking. The [`-C prefer-dynamic`
flag][prefer-dynamic] may be used to influence which is used.

If the same crate name is specified with and without a path, the one with the
path is used and the pathless flag has no effect.

[extern prelude]: ../reference/items/extern-crates.html#extern-prelude
[prefer-dynamic]: codegen-options/index.md#prefer-dynamic

<a id="option-sysroot"></a>
## `--sysroot`: Override the system root
Expand Down
17 changes: 3 additions & 14 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1800,7 +1800,7 @@ pub fn rustc_optgroups() -> Vec<RustcOptGroup> {
"",
"extern",
"Specify where an external rust library is located",
"NAME=PATH",
"NAME[=PATH]",
),
opt::multi_s(
"",
Expand Down Expand Up @@ -2164,7 +2164,6 @@ fn collect_print_requests(
cg: &mut CodegenOptions,
dopts: &mut DebuggingOptions,
matches: &getopts::Matches,
is_unstable_enabled: bool,
error_format: ErrorOutputType,
) -> Vec<PrintRequest> {
let mut prints = Vec::<PrintRequest>::new();
Expand Down Expand Up @@ -2206,7 +2205,7 @@ fn collect_print_requests(
"tls-models" => PrintRequest::TlsModels,
"native-static-libs" => PrintRequest::NativeStaticLibs,
"target-spec-json" => {
if is_unstable_enabled {
if dopts.unstable_options {
PrintRequest::TargetSpec
} else {
early_error(
Expand Down Expand Up @@ -2370,7 +2369,6 @@ fn parse_externs(
matches: &getopts::Matches,
debugging_opts: &DebuggingOptions,
error_format: ErrorOutputType,
is_unstable_enabled: bool,
) -> Externs {
if matches.opt_present("extern-private") && !debugging_opts.unstable_options {
early_error(
Expand All @@ -2392,13 +2390,6 @@ fn parse_externs(
let name = parts.next().unwrap_or_else(||
early_error(error_format, "--extern value must not be empty"));
let location = parts.next().map(|s| s.to_string());
if location.is_none() && !is_unstable_enabled {
early_error(
error_format,
"the `-Z unstable-options` flag must also be passed to \
enable `--extern crate_name` without `=path`",
);
};

let entry = externs
.entry(name.to_owned())
Expand Down Expand Up @@ -2483,12 +2474,10 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {
);
}

let is_unstable_enabled = nightly_options::is_unstable_enabled(matches);
let prints = collect_print_requests(
&mut cg,
&mut debugging_opts,
matches,
is_unstable_enabled,
error_format,
);

Expand Down Expand Up @@ -2521,7 +2510,7 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {
);
}

let externs = parse_externs(matches, &debugging_opts, error_format, is_unstable_enabled);
let externs = parse_externs(matches, &debugging_opts, error_format);

let crate_name = matches.opt_str("crate-name");

Expand Down
4 changes: 0 additions & 4 deletions src/librustdoc/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -615,10 +615,6 @@ fn parse_externs(matches: &getopts::Matches) -> Result<Externs, String> {
let mut parts = arg.splitn(2, '=');
let name = parts.next().ok_or("--extern value must not be empty".to_string())?;
let location = parts.next().map(|s| s.to_string());
if location.is_none() && !nightly_options::is_unstable_enabled(matches) {
return Err("the `-Z unstable-options` flag must also be passed to \
enable `--extern crate_name` without `=path`".to_string());
}
let name = name.to_string();
// For Rustdoc purposes, we can treat all externs as public
externs.entry(name)
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ fn opts() -> Vec<RustcOptGroup> {
}),
stable("cfg", |o| o.optmulti("", "cfg", "pass a --cfg to rustc", "")),
stable("extern", |o| {
o.optmulti("", "extern", "pass an --extern to rustc", "NAME=PATH")
o.optmulti("", "extern", "pass an --extern to rustc", "NAME[=PATH]")
}),
unstable("extern-html-root-url", |o| {
o.optmulti("", "extern-html-root-url",
Expand Down
4 changes: 3 additions & 1 deletion src/test/run-make-fulldeps/extern-flag-fun/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ all:
$(RUSTC) bar.rs --crate-type=rlib
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only test?

Copy link
Contributor Author

@ehuss ehuss Sep 30, 2019

Choose a reason for hiding this comment

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

extern-flag-fun is not actually testing this feature. It is a 5-year old test from when --extern was first added. The removed --extern hello line was originally there to verify that it would error due to the missing path. When pathless --extern was added, the error changed to "missing -Z unstable-options". With this PR, the hello line no longer fails (and thus breaks the test), due to -L flags injected by the makefile. I figured it no longer fit the spirit of the test.

I have added some tests.

$(RUSTC) bar.rs --crate-type=rlib -C extra-filename=-a
$(RUSTC) bar-alt.rs --crate-type=rlib
$(RUSTC) foo.rs --extern hello && exit 1 || exit 0
$(RUSTC) foo.rs --extern bar=no-exist && exit 1 || exit 0
$(RUSTC) foo.rs --extern bar=foo.rs && exit 1 || exit 0
$(RUSTC) foo.rs \
Expand All @@ -15,3 +14,6 @@ all:
--extern bar=$(TMPDIR)/libbar.rlib \
--extern bar=$(TMPDIR)/libbar-a.rlib
$(RUSTC) foo.rs --extern bar=$(TMPDIR)/libbar.rlib
# Try to be sneaky and load a private crate from with a non-private name.
$(RUSTC) rustc.rs -Zforce-unstable-if-unmarked --crate-type=rlib
$(RUSTC) gated_unstable.rs --extern alloc=$(TMPDIR)/librustc.rlib 2>&1 | $(CGREP) 'rustc_private'
3 changes: 3 additions & 0 deletions src/test/run-make-fulldeps/extern-flag-fun/gated_unstable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
extern crate alloc;

fn main() {}
1 change: 1 addition & 0 deletions src/test/run-make-fulldeps/extern-flag-fun/rustc.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub fn foo() {}
18 changes: 18 additions & 0 deletions src/test/run-make-fulldeps/extern-flag-pathless/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
-include ../tools.mk

# Test mixing pathless --extern with paths.

all:
$(RUSTC) bar-static.rs --crate-name=bar --crate-type=rlib
$(RUSTC) bar-dynamic.rs --crate-name=bar --crate-type=dylib -C prefer-dynamic
# rlib preferred over dylib
$(RUSTC) foo.rs --extern bar
$(call RUN,foo) | $(CGREP) 'static'
$(RUSTC) foo.rs --extern bar=$(TMPDIR)/libbar.rlib --extern bar
$(call RUN,foo) | $(CGREP) 'static'
# explicit --extern overrides pathless
$(RUSTC) foo.rs --extern bar=$(call DYLIB,bar) --extern bar
$(call RUN,foo) | $(CGREP) 'dynamic'
# prefer-dynamic does what it says
$(RUSTC) foo.rs --extern bar -C prefer-dynamic
$(call RUN,foo) | $(CGREP) 'dynamic'
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub fn f() {
println!("dynamic");
}
3 changes: 3 additions & 0 deletions src/test/run-make-fulldeps/extern-flag-pathless/bar-static.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub fn f() {
println!("static");
}
3 changes: 3 additions & 0 deletions src/test/run-make-fulldeps/extern-flag-pathless/foo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
fn main() {
bar::f();
}
3 changes: 1 addition & 2 deletions src/test/run-make-fulldeps/save-analysis-rfc2126/Makefile
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
-include ../tools.mk

all: extern_absolute_paths.rs krate2
$(RUSTC) extern_absolute_paths.rs -Zsave-analysis --edition=2018 \
-Z unstable-options --extern krate2
$(RUSTC) extern_absolute_paths.rs -Zsave-analysis --edition=2018 --extern krate2
cat $(TMPDIR)/save-analysis/extern_absolute_paths.json | "$(PYTHON)" validate_json.py

krate2: krate2.rs
Expand Down
2 changes: 1 addition & 1 deletion src/test/rustdoc/inline_cross/use_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// aux-build:use_crate_2.rs
// build-aux-docs
// edition:2018
// compile-flags:--extern use_crate --extern use_crate_2 -Z unstable-options
// compile-flags:--extern use_crate --extern use_crate_2

// During the buildup to Rust 2018, rustdoc would eagerly inline `pub use some_crate;` as if it
// were a module, so we changed it to make `pub use`ing crate roots remain as a `pub use` statement
Expand Down
10 changes: 10 additions & 0 deletions src/test/ui-fulldeps/pathless-extern-unstable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// ignore-stage1
// edition:2018
// compile-flags:--extern rustc
Centril marked this conversation as resolved.
Show resolved Hide resolved

// Test that `--extern rustc` fails with `rustc_private`.

pub use rustc;
//~^ ERROR use of unstable library feature 'rustc_private'

fn main() {}
12 changes: 12 additions & 0 deletions src/test/ui-fulldeps/pathless-extern-unstable.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0658]: use of unstable library feature 'rustc_private': this crate is being loaded from the sysroot, an unstable location; did you mean to load this crate from crates.io via `Cargo.toml` instead?
--> $DIR/pathless-extern-unstable.rs:7:9
|
LL | pub use rustc;
| ^^^^^
|
= note: for more information, see https://github.com/rust-lang/rust/issues/27812
= help: add `#![feature(rustc_private)]` to the crate attributes to enable

error: aborting due to previous error

For more information about this error, try `rustc --explain E0658`.
9 changes: 9 additions & 0 deletions src/test/ui/pathless-extern-ok.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// edition:2018
// compile-flags:--extern alloc
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this achieved by whitelist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a sense, yes. The crate must be marked with #![stable], otherwise the -Zforce-unstable-if-unmarked makes it unstable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah... so just for my education, what happens if -Z force-unstable-if-unmarked isn't there? (And it cannot be there on stable when you are not using cargo since it's a -Z flag?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will successfully link whatever library is in the given path. That library will be added to the extern prelude with the identifier "alloc".

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that a problem then if you are not using cargo since it would allow you to use unstable stuff on stable? I'm rather surprised that -Z force-unstable-if-unmarked isn't the default and that you need a flag to opt-out for sysroot crates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm not understanding the question.

On stable rustc, you cannot access any crates that are marked "unstable". Either from --extern foo or --extern foo=path/to/sysroot/libfoo.rlib or extern crate foo; or -L path/to/unstable/crates.

On nightly, you can include the attribute #![feature(rustc_private)] to override this check.

All of the sysroot crates are built with the -Zforce-unstable-if-unmarked flag.

The opt-in nature is just saying #![stable] crates are OK to access, even with the -Zforce-unstable-if-unmarked flag (that is the "if unmarked" part). I believe the only crates marked stable are core, std, alloc, and proc_macro.

In theory, anyone can compile any crate using unstable features using nightly, and then load those crates from stable. In practice, you can't (due to rustc version checking) or clearly something you shouldn't do (like set RUSTC_BOOTSTRAP env var).

Can you say more how you think that can be circumvented?

Copy link
Contributor

Choose a reason for hiding this comment

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

All of the sysroot crates are built with the -Zforce-unstable-if-unmarked flag.

Oh! -- I missed this part. I thought you meant that when the sysroot crate is used, unless there is a -Zforce..., then it won't be gated... but I see now that this is about when the crate is built, not used. Now it clicks. Maybe this conversation should be distilled into the rustc guide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// build-pass

// Test that `--extern alloc` will load from the sysroot without error.

fn main() {
let _: Vec<i32> = alloc::vec::Vec::new();
}