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

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Sep 28, 2019

This stabilizes the --extern flag without a path, implemented in #54116.

This flag is used to add a crate that may be found in the search path to the extern prelude. The intent of stabilizing this now is to change Cargo to emit this flag for proc_macro when building a proc-macro crate. This will allow the ability to elide extern crate proc_macro; for proc-macros, one of the few places where it is still necessary.

It is intended that Cargo may also use this flag for other cases in the future as part of the std-aware work. There will likely be some kind of syntax where users may declare dependencies on other crates (such as alloc), and Cargo will use this flag so that they may be used like any other crate. At this time there are no short-term plans to use it for anything other than proc-macro.

This will not help for non-proc-macro crates that use proc_macro, which I believe is not too common?

An alternate approach for proc-macro is to use the meta crate, but from my inquiries there doesn't appear to be anyone interested in pushing that forward. The meta crate also doesn't help with things like alloc or test.

cc #57288

@Centril Centril added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. labels Sep 28, 2019
@Centril Centril added this to the 1.40 milestone Sep 28, 2019
@@ -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.


* `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.

@withoutboats
Copy link
Contributor

We discussed this in the lang team meeting. In general, we are comfortable with stabilizing this feature. We would like some more commentary from other people with more knowledge of build systems, the sysroot, etc (lang team members with more relevant experience here weren't available today), mainly about what possible hazards there could be with this feature.

In terms of the larger goal this is trying to solve (eliminating the remaining use cases for extern crate), we had some other thoughts. This is really something that has been dropped on the floor, and no one seems available to shepherd it. It seems good to make proc_macro available in crates with proc-macro marked in lib, and this plus the associated change to cargo seems like a great solution to that problem. In the long term, we would like to reorganize the proc_macro APIs under meta and deprecate proc_macro, but - again - no one is shepherding that (as you commented).

However, it's not clear if this will be a step to solving the no_std and test problems, which have some important differences (most importantly, currently there's no clear way the need for these crates is marked in the toml). We think a full RFC process would be necessary to figure out the interface to access these crates without using extern crate.

@ehuss
Copy link
Contributor Author

ehuss commented Oct 3, 2019

We think a full RFC process would be necessary to figure out the interface to access these crates without using extern crate.

Absolutely. That is the intent of rust-lang/wg-cargo-std-aware#5 to produce an RFC with the necessary Cargo.toml syntax to make these dependencies explicit, and is the next major milestone I plan to tackle.

@@ -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.

src/test/ui-fulldeps/pathless-extern-unstable.rs Outdated Show resolved Hide resolved
@withoutboats
Copy link
Contributor

Absolutely. That is the intent of rust-lang/wg-cargo-std-aware#5 to produce an RFC with the necessary Cargo.toml syntax to make these dependencies explicit, and is the next major milestone I plan to tackle.

As a note, one reason I think we have not been enthusiastic on this is for the primary use case - core, alloc - there's a lot of good reason to deprecate these crates and move to a system that works more like feature flags on a single crate (so we can add impls without orphan issues, for example). However, this is a big and complicated refactor of the standard library and no one is driving it.

@ehuss ehuss force-pushed the stabilize-bare-extern branch from c6ad892 to 6ba1de4 Compare October 8, 2019 16:45
@nikomatsakis nikomatsakis removed I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 10, 2019
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

Dear compiler team,

I propose that we merge this PR. It adds a --extern foo flag which adds a crate foo from the syspath (in contrast to --extern foo=path which adds the crate foo from a specific path). The lang team has signed off on the general concept.

@rfcbot
Copy link

rfcbot commented Oct 10, 2019

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 10, 2019
@Centril Centril removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Oct 11, 2019
@ehuss
Copy link
Contributor Author

ehuss commented Oct 15, 2019

I just realized that this does not support crate renaming (the equivalent of extern crate test as std_test). Some options:

  1. Delay this stabilization until it is supported.
  2. Add support later through some extension of the existing CLI syntax.
  3. Don't support renaming.

It looks like it would be relatively easy to add. I'm curious what anyone else thinks. I'd also like to hear if anyone would have any ideas on the cli syntax.

I think I'd like to lean towards 1 or 2, as I think it would be an odd wart not to support it. And if others agree, maybe waiting would be prudent?

@petrochenkov
Copy link
Contributor

2 is ok, IMO.

The semantics are clear

--extern foo=PATH // search by exact path `PATH` and add to extern prelude as `foo`
--extern foo???bar // search by name `bar` in the search directories and add to extern prelude as `foo`
--extern foo // sugar for `--extern foo???foo`

we only need to review the syntactic space so that --extern foo???bar is never ambiguous with --extern foo=PATH and perhaps with --extern:modifier foo[=PATH|???bar].

such as within the sysroot or via the `-L` flag.

The same crate name may be specified multiple times for different crate types.
For loading metadata, `rlib` takes precedence over `rmeta`, which takes
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 want to guarantee that? Does any tools rely on that? (The "rlib ->rmeta -> dylib" order for metadata.)

Hypothetically, what if we want to change it to "rmeta -> rlib -> dylib"?
rmeta is smaller, easier to obtain, perhaps faster to read, etc - same reasons why rlib is preferred to dylib, basically. Perhaps we can exploit that somehow.
Then maybe it better stay unspecified in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I have removed it. I am not aware of any tools relying on that behavior, and I think it would be a little strange to do so.

@ehuss ehuss force-pushed the stabilize-bare-extern branch from 6ba1de4 to 098ffd9 Compare October 16, 2019 16:55
@JohnCSimon JohnCSimon added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2019
@JohnCSimon
Copy link
Member

Ping from triage:
This PR seems to be waiting on review @frewsxcv @petrochenkov
Cc: @ehuss
Thanks

@frewsxcv frewsxcv removed their assignment Oct 26, 2019
@frewsxcv
Copy link
Member

I'm not familiar with any of this code; can someone else review it?

@Centril
Copy link
Contributor

Centril commented Oct 26, 2019

Ping @Zoxc @michaelwoerister, and @nagisa.

r? @eddyb or maybe @petrochenkov

@Centril
Copy link
Contributor

Centril commented Nov 7, 2019

^-- Failed in #66185 (comment), @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 7, 2019
@GuillaumeGomez
Copy link
Member

Please fix the id duplicate then it can get r+ again.

@bors: r-

@Centril Centril modified the milestones: 1.40, 1.41 Nov 7, 2019
@ehuss ehuss force-pushed the stabilize-bare-extern branch from 098ffd9 to ee459c6 Compare November 7, 2019 15:21
@ehuss
Copy link
Contributor Author

ehuss commented Nov 7, 2019

@bors r=eddyb

Strange that github/bors did not have a merge conflict, since I got one locally.

@bors
Copy link
Contributor

bors commented Nov 7, 2019

📌 Commit ee459c6 has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2019
@bors
Copy link
Contributor

bors commented Nov 8, 2019

⌛ Testing commit ee459c6 with merge d257440...

bors added a commit that referenced this pull request Nov 8, 2019
Stabilize --extern flag without a path.

This stabilizes the `--extern` flag without a path, implemented in #54116.

This flag is used to add a crate that may be found in the search path to the extern prelude. The intent of stabilizing this now is to change Cargo to emit this flag for `proc_macro` when building a proc-macro crate. This will allow the ability to elide `extern crate proc_macro;` for proc-macros, one of the few places where it is still necessary.

It is intended that Cargo may also use this flag for other cases in the future as part of the [std-aware work](https://github.com/rust-lang/wg-cargo-std-aware/). There will likely be some kind of syntax where users may declare dependencies on other crates (such as `alloc`), and Cargo will use this flag so that they may be used like any other crate. At this time there are no short-term plans to use it for anything other than proc-macro.

This will not help for non-proc-macro crates that use `proc_macro`, which I believe is not too common?

An alternate approach for proc-macro is to use the `meta` crate, but from my inquiries there doesn't appear to be anyone interested in pushing that forward. The `meta` crate also doesn't help with things like `alloc` or `test`.

cc #57288
@bors
Copy link
Contributor

bors commented Nov 8, 2019

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing d257440 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 8, 2019
@bors bors merged commit ee459c6 into rust-lang:master Nov 8, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #64882!

Tested on commit d257440.
Direct link to PR: #64882

💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Nov 8, 2019
Tested on commit rust-lang/rust@d257440.
Direct link to PR: <rust-lang/rust#64882>

💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).
bors added a commit to rust-lang/cargo that referenced this pull request Dec 16, 2019
Add proc_macro to the extern prelude.

This makes it so that a proc-macro library can use the `proc_macro` crate without the `extern crate proc_macro;` item on the 2018 edition.  This is the Cargo half of rust-lang/rust#64882.
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 17, 2020
Version 1.41.0 (2020-01-30)
===========================

Language
--------

- [You can now pass type parameters to foreign items when implementing
  traits.][65879] E.g. You can now write `impl<T> From<Foo> for Vec<T> {}`.
- [You can now arbitrarily nest receiver types in the `self` position.][64325] E.g. you can
  now write `fn foo(self: Box<Box<Self>>) {}`. Previously only `Self`, `&Self`,
  `&mut Self`, `Arc<Self>`, `Rc<Self>`, and `Box<Self>` were allowed.
- [You can now use any valid identifier in a `format_args` macro.][66847]
  Previously identifiers starting with an underscore were not allowed.
- [Visibility modifiers (e.g. `pub`) are now syntactically allowed on trait items and
  enum variants.][66183] These are still rejected semantically, but
  can be seen and parsed by procedural macros and conditional compilation.

Compiler
--------

- [Rustc will now warn if you have unused loop `'label`s.][66325]
- [Removed support for the `i686-unknown-dragonfly` target.][67255]
- [Added tier 3 support\* for the `riscv64gc-unknown-linux-gnu` target.][66661]
- [You can now pass an arguments file passing the `@path` syntax
  to rustc.][66172] Note that the format differs somewhat from what is
  found in other tooling; please see [the documentation][argfile-docs] for
  more information.
- [You can now provide `--extern` flag without a path, indicating that it is
  available from the search path or specified with an `-L` flag.][64882]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

[argfile-docs]: https://doc.rust-lang.org/nightly/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path

Libraries
---------

- [The `core::panic` module is now stable.][66771] It was already stable
  through `std`.
- [`NonZero*` numerics now implement `From<NonZero*>` if it's a smaller integer
  width.][66277] E.g. `NonZeroU16` now implements `From<NonZeroU8>`.
- [`MaybeUninit<T>` now implements `fmt::Debug`.][65013]

Stabilized APIs
---------------

- [`Result::map_or`]
- [`Result::map_or_else`]
- [`std::rc::Weak::weak_count`]
- [`std::rc::Weak::strong_count`]
- [`std::sync::Weak::weak_count`]
- [`std::sync::Weak::strong_count`]

Cargo
-----

- [Cargo will now document all the private items for binary crates
  by default.][cargo/7593]
- [`cargo-install` will now reinstall the package if it detects that it is out
  of date.][cargo/7560]
- [Cargo.lock now uses a more git friendly format that should help to reduce
  merge conflicts.][cargo/7579]
- [You can now override specific dependencies's build settings][cargo/7591] E.g.
  `[profile.dev.overrides.image] opt-level = 2` sets the `image` crate's
  optimisation level to `2` for debug builds. You can also use
  `[profile.<profile>.build_overrides]` to override build scripts and
  their dependencies.

Misc
----

- [You can now specify `edition` in documentation code blocks to compile the block
  for that edition.][66238] E.g. `edition2018` tells rustdoc that the code sample
  should be compiled the 2018 edition of Rust.
- [You can now provide custom themes to rustdoc with `--theme`, and check the
  current theme with `--check-theme`.][54733]
- [You can use `#[cfg(doc)]` to compile an item when building documentation.][61351]

Compatibility Notes
-------------------

- [As previously announced 1.41.0 will be the last tier 1 release for 32-bit
  Apple targets.][apple-32bit-drop] This means that the source code is still
  available to build, but the targets are no longer being tested and release
  binaries for those platforms will no longer be distributed by the Rust project.
  Please refer to the linked blog post for more information.

[54733]: rust-lang/rust#54733
[61351]: rust-lang/rust#61351
[67255]: rust-lang/rust#67255
[66661]: rust-lang/rust#66661
[66771]: rust-lang/rust#66771
[66847]: rust-lang/rust#66847
[66238]: rust-lang/rust#66238
[66277]: rust-lang/rust#66277
[66325]: rust-lang/rust#66325
[66172]: rust-lang/rust#66172
[66183]: rust-lang/rust#66183
[65879]: rust-lang/rust#65879
[65013]: rust-lang/rust#65013
[64882]: rust-lang/rust#64882
[64325]: rust-lang/rust#64325
[cargo/7560]: rust-lang/cargo#7560
[cargo/7579]: rust-lang/cargo#7579
[cargo/7591]: rust-lang/cargo#7591
[cargo/7593]: rust-lang/cargo#7593
[`Result::map_or_else`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.map_or_else
[`Result::map_or`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.map_or
[`std::rc::Weak::weak_count`]: https://doc.rust-lang.org/std/rc/struct.Weak.html#method.weak_count
[`std::rc::Weak::strong_count`]: https://doc.rust-lang.org/std/rc/struct.Weak.html#method.strong_count
[`std::sync::Weak::weak_count`]: https://doc.rust-lang.org/std/sync/struct.Weak.html#method.weak_count
[`std::sync::Weak::strong_count`]: https://doc.rust-lang.org/std/sync/struct.Weak.html#method.strong_count
[apple-32bit-drop]: https://blog.rust-lang.org/2020/01/03/reducing-support-for-32-bit-apple-targets.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.