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

Change --crate-type metadata to --emit=metadata #38571

Merged
merged 4 commits into from Dec 29, 2016

Conversation

@nrc
Copy link
Member

commented Dec 23, 2016

WIP

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Dec 23, 2016

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@nrc nrc force-pushed the nrc:emit-metadata-change branch from be65826 to 8ee4860 Dec 23, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 25, 2016

Sounds like a great solution to the metadata-lib/metadata-bin problem to me!

I think we may want to keep --crate-type metadata for a cycle or so though as Cargo's currently relying on it?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2016

👍

@nrc

This comment has been minimized.

Copy link
Member Author

commented Dec 27, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 27, 2016

Looks good to me! My only comment is that I think there's a few places where --emit link,metadata will go awry. That is, I think that'd only emit metadata rather than both.

Otherwise I'm curious what others think about the changes to --crate-type and trying to preserve backcompat here. I'm leaning towards reverting the addition of --crate-type metadata on beta as well as cargo check on beta. We can land this PR, fix Cargo, fix the other assorted Cargo bugs associated with cargo check, and then let it ride the trains through on 1.16.

If we take that route I think we can avoid the --crate-type deprecation warning here and just make the change, but if we don't take that route then we'll need to leave the deprecation warning in.

@nrc nrc force-pushed the nrc:emit-metadata-change branch from 28decd4 to bc4b42f Dec 28, 2016

@nrc

This comment has been minimized.

Copy link
Member Author

commented Dec 28, 2016

My only comment is that I think there's a few places where --emit link,metadata will go awry. That is, I think that'd only emit metadata rather than both.

This was intentional (although I admit it is sub-optimal). Basically, it seemed very difficult to make the compiler backend emit multiple artefacts, so I chose that --emit=metadata should override any other emit kinds (this only affects obj/link/other emit kinds that reach the compiler backend). This seems sensible - there is no point in emitting metadata and any other backend artefact, since the latter will always include the metadata. We could warn about this, I guess?

I was imagining pushing this to beta, rather than reverting --crate-type=metadata and landing this on nightly, but I don't mind the more conservative approach. I think we need the deprecated --crate-type just until we land a fix to Cargo (which I'm currently working on) otherwise we'll get Cargo test breakage (which I think will block this PR landing). (Oh, we could remove cargo check first, but that makes landing the Cargo fix a little bit more effort).

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

@nrc I'd be wary of having --emit metadata perform differently than other requests like --emit dep-info,link. Although it may be rare, we'll inevitably receive a bug report about --emit metadata,link not working and I think it'd be good to head off that report here.

I think it also may not be too hard to support, right? There's a few clauses that short-circuit lots of extra work throughout this PR, but couldn't those clauses just change to "short circuit if we're only producing metadata" ? That way we'd just fall through all the normal code paths and if you happened to pass --emit metadata,link we'd fall through and at one point emit metadata in addition to final binary.

Let's discuss the strategy for beta on a future PR (or on a discussion focused on beta-nomination of this PR), though, don't want to bog it down!

@nrc

This comment has been minimized.

Copy link
Member Author

commented Dec 28, 2016

"short circuit if we're only producing metadata"

This doesn't work because we want to ignore emit kinds which are emitted before we hit the backend, e.g., --emit=metadata,dep-info (which Cargo uses a lot) should trigger these paths. It should be possible to handle this, but it is not trivial.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

@nrc perhaps the checks could be "if link is present, take this code path"?

I'll comment on the branches I'm thinking about specifically

@@ -103,6 +103,10 @@ pub fn calculate(sess: &session::Session) {

fn calculate_type(sess: &session::Session,
ty: config::CrateType) -> DependencyList {
if sess.opts.output_types.contains_key(&config::OutputType::Metadata) {

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Dec 28, 2016

Member

Here I think we could just say that if the outputs contain link we can take the paths below, right?

@@ -1558,7 +1566,13 @@ pub fn parse_crate_types_from_list(list_list: Vec<String>) -> Result<Vec<CrateTy
"cdylib" => CrateTypeCdylib,
"bin" => CrateTypeExecutable,
"proc-macro" => CrateTypeProcMacro,
"metadata" => CrateTypeMetadata,
// FIXME(#38640) remove this when Cargo is fixed.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Dec 28, 2016

Member

Given our discussion today during triage, I think we can remove this entirely

@@ -191,7 +191,8 @@ pub fn link_binary(sess: &Session,
let mut out_filenames = Vec::new();
for &crate_type in sess.crate_types.borrow().iter() {
// Ignore executable crates if we have -Z no-trans, as they will error.
if sess.opts.debugging_opts.no_trans &&
if (sess.opts.debugging_opts.no_trans ||
sess.opts.output_types.contains_key(&OutputType::Metadata)) &&

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Dec 28, 2016

Member

Here we could just check for !contains(Exe), right?


if outputs.outputs.contains_key(&OutputType::Metadata) {
return outputs.out_directory.join(&format!("lib{}.rmeta", libname));
}

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Dec 28, 2016

Member

This is where it may get a little tricky perhaps? It seems that the request for a metadata filename may want to be plumbed farther throughout the backend?

link_natively(sess, crate_type, &objects, &out_filename, trans,
outputs, tmpdir.path());
if outputs.outputs.contains_key(&OutputType::Metadata) {
emit_metadata(sess, trans, &out_filename);

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Dec 28, 2016

Member

These could just be two if statements, right?

if metadata_requested {
    emit_metadata(...);
}

if link_requested {
    ...
}

@nrc nrc force-pushed the nrc:emit-metadata-change branch from bc4b42f to b059a80 Dec 29, 2016

@nrc

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2016

r? @alexcrichton can handle --emit=metadata + something else now.

Whether we remove the CrateTypeMetadata variant depends on how we handle Cargo, and I'm not sure where we're at there - I think it would be easier to remove check on beta and leave it as is on nightly (I have a PR ready for that once this lands), then we can fixup nightly pretty quickly, but that would be easier based on top of current Cargo, rather than Cargo without check.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Dec 29, 2016

@bors: r+

Ok looks good to me! We can remove --crate-type=metadata as soon as Cargo is updated.

@bors

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2016

📌 Commit b059a80 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2016

⌛️ Testing commit b059a80 with merge e571f2d...

bors added a commit that referenced this pull request Dec 29, 2016
Auto merge of #38571 - nrc:emit-metadata-change, r=alexcrichton
Change --crate-type metadata to --emit=metadata

WIP
@bors

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2016

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing e571f2d to master...

@bors bors merged commit b059a80 into rust-lang:master Dec 29, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
bors added a commit to rust-lang/cargo that referenced this pull request Jan 3, 2017
Auto merge of #3468 - nrc:metadata-emit, r=alexcrichton
cargo check: use --emit=metadata rather than --crate-type=metadata

Requires rust-lang/rust#38571 (don't land before that does)

r? @alexcrichton
nrc added a commit to nrc/cargo that referenced this pull request Jan 3, 2017
nrc added a commit to nrc/cargo that referenced this pull request Jan 3, 2017
nrc added a commit to nrc/cargo that referenced this pull request Jan 4, 2017
bors added a commit to rust-lang/cargo that referenced this pull request Jan 5, 2017
Auto merge of #3468 - nrc:metadata-emit, r=alexcrichton
cargo check: use --emit=metadata rather than --crate-type=metadata

Requires rust-lang/rust#38571 (don't land before that does)

r? @alexcrichton
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Mar 20, 2017
jperkin
Update lang/rust to 1.16.0. Changes since 1.15.1:
Version 1.16.0 (2017-03-16)
===========================

Language
--------

* Lifetimes in statics and consts default to `'static`. [RFC 1623]
* [The compiler's `dead_code` lint now accounts for type aliases][38051].
* [Uninhabitable enums (those without any variants) no longer permit wildcard
  match patterns][38069]
* [Clean up semantics of `self` in an import list][38313]
* [`Self` may appear in `impl` headers][38920]
* [`Self` may appear in struct expressions][39282]

Compiler
--------

* [`rustc` now supports `--emit=metadata`, which causes rustc to emit
  a `.rmeta` file containing only crate metadata][38571]. This can be
  used by tools like the Rust Language Service to perform
  metadata-only builds.
* [Levenshtein based typo suggestions now work in most places, while
  previously they worked only for fields and sometimes for local
  variables][38927]. Together with the overhaul of "no
  resolution"/"unexpected resolution" errors (#[38154]) they result in
  large and systematic improvement in resolution diagnostics.
* [Fix `transmute::<T, U>` where `T` requires a bigger alignment than
  `U`][38670]
* [rustc: use -Xlinker when specifying an rpath with ',' in it][38798]
* [`rustc` no longer attempts to provide "consider using an explicit
  lifetime" suggestions][37057]. They were inaccurate.

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

* [`VecDeque::truncate`]
* [`VecDeque::resize`]
* [`String::insert_str`]
* [`Duration::checked_add`]
* [`Duration::checked_sub`]
* [`Duration::checked_div`]
* [`Duration::checked_mul`]
* [`str::replacen`]
* [`str::repeat`]
* [`SocketAddr::is_ipv4`]
* [`SocketAddr::is_ipv6`]
* [`IpAddr::is_ipv4`]
* [`IpAddr::is_ipv6`]
* [`Vec::dedup_by`]
* [`Vec::dedup_by_key`]
* [`Result::unwrap_or_default`]
* [`<*const T>::wrapping_offset`]
* [`<*mut T>::wrapping_offset`]
* `CommandExt::creation_flags`
* [`File::set_permissions`]
* [`String::split_off`]

Libraries
---------

* [`[T]::binary_search` and `[T]::binary_search_by_key` now take
  their argument by `Borrow` parameter][37761]
* [All public types in std implement `Debug`][38006]
* [`IpAddr` implements `From<Ipv4Addr>` and `From<Ipv6Addr>`][38327]
* [`Ipv6Addr` implements `From<[u16; 8]>`][38131]
* [Ctrl-Z returns from `Stdin.read()` when reading from the console on
  Windows][38274]
* [std: Fix partial writes in `LineWriter`][38062]
* [std: Clamp max read/write sizes on Unix][38062]
* [Use more specific panic message for `&str` slicing errors][38066]
* [`TcpListener::set_only_v6` is deprecated][38304]. This
  functionality cannot be achieved in std currently.
* [`writeln!`, like `println!`, now accepts a form with no string
  or formatting arguments, to just print a newline][38469]
* [Implement `iter::Sum` and `iter::Product` for `Result`][38580]
* [Reduce the size of static data in `std_unicode::tables`][38781]
* [`char::EscapeDebug`, `EscapeDefault`, `EscapeUnicode`,
  `CaseMappingIter`, `ToLowercase`, `ToUppercase`, implement
  `Display`][38909]
* [`Duration` implements `Sum`][38712]
* [`String` implements `ToSocketAddrs`][39048]

Cargo
-----

* [The `cargo check` command does a type check of a project without
  building it][cargo/3296]
* [crates.io will display CI badges from Travis and AppVeyor, if
  specified in Cargo.toml][cargo/3546]
* [crates.io will display categories listed in Cargo.toml][cargo/3301]
* [Compilation profiles accept integer values for `debug`, in addition
  to `true` and `false`. These are passed to `rustc` as the value to
  `-C debuginfo`][cargo/3534]
* [Implement `cargo --version --verbose`][cargo/3604]
* [All builds now output 'dep-info' build dependencies compatible with
  make and ninja][cargo/3557]
* [Build all workspace members with `build --all`][cargo/3511]
* [Document all workspace members with `doc --all`][cargo/3515]
* [Path deps outside workspace are not members][cargo/3443]

Misc
----

* [`rustdoc` has a `--sysroot` argument that, like `rustc`, specifies
  the path to the Rust implementation][38589]
* [The `armv7-linux-androideabi` target no longer enables NEON
  extensions, per Google's ABI guide][38413]
* [The stock standard library can be compiled for Redox OS][38401]
* [Rust has initial SPARC support][38726]. Tier 3. No builds
  available.
* [Rust has experimental support for Nvidia PTX][38559]. Tier 3. No
  builds available.
* [Fix backtraces on i686-pc-windows-gnu by disabling FPO][39379]

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

* [Uninhabitable enums (those without any variants) no longer permit wildcard
  match patterns][38069]
* In this release, references to uninhabited types can not be
  pattern-matched. This was accidentally allowed in 1.15.
* [The compiler's `dead_code` lint now accounts for type aliases][38051].
* [Ctrl-Z returns from `Stdin.read()` when reading from the console on
  Windows][38274]
* [Clean up semantics of `self` in an import list][38313]

[37057]: rust-lang/rust#37057
[37761]: rust-lang/rust#37761
[38006]: rust-lang/rust#38006
[38051]: rust-lang/rust#38051
[38062]: rust-lang/rust#38062
[38062]: rust-lang/rust#38622
[38066]: rust-lang/rust#38066
[38069]: rust-lang/rust#38069
[38131]: rust-lang/rust#38131
[38154]: rust-lang/rust#38154
[38274]: rust-lang/rust#38274
[38304]: rust-lang/rust#38304
[38313]: rust-lang/rust#38313
[38314]: rust-lang/rust#38314
[38327]: rust-lang/rust#38327
[38401]: rust-lang/rust#38401
[38413]: rust-lang/rust#38413
[38469]: rust-lang/rust#38469
[38559]: rust-lang/rust#38559
[38571]: rust-lang/rust#38571
[38580]: rust-lang/rust#38580
[38589]: rust-lang/rust#38589
[38670]: rust-lang/rust#38670
[38712]: rust-lang/rust#38712
[38726]: rust-lang/rust#38726
[38781]: rust-lang/rust#38781
[38798]: rust-lang/rust#38798
[38909]: rust-lang/rust#38909
[38920]: rust-lang/rust#38920
[38927]: rust-lang/rust#38927
[39048]: rust-lang/rust#39048
[39282]: rust-lang/rust#39282
[39379]: rust-lang/rust#39379
[`<*const T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset
[`<*mut T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset
[`Duration::checked_add`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_add
[`Duration::checked_div`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_div
[`Duration::checked_mul`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_mul
[`Duration::checked_sub`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_sub
[`File::set_permissions`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.set_permissions
[`IpAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv4
[`IpAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv6
[`Result::unwrap_or_default`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default
[`SocketAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv4
[`SocketAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv6
[`String::insert_str`]: https://doc.rust-lang.org/std/string/struct.String.html#method.insert_str
[`String::split_off`]: https://doc.rust-lang.org/std/string/struct.String.html#method.split_off
[`Vec::dedup_by_key`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by_key
[`Vec::dedup_by`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by
[`VecDeque::resize`]:  https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.resize
[`VecDeque::truncate`]: https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.truncate
[`str::repeat`]: https://doc.rust-lang.org/std/primitive.str.html#method.repeat
[`str::replacen`]: https://doc.rust-lang.org/std/primitive.str.html#method.replacen
[cargo/3296]: rust-lang/cargo#3296
[cargo/3301]: rust-lang/cargo#3301
[cargo/3443]: rust-lang/cargo#3443
[cargo/3511]: rust-lang/cargo#3511
[cargo/3515]: rust-lang/cargo#3515
[cargo/3534]: rust-lang/cargo#3534
[cargo/3546]: rust-lang/cargo#3546
[cargo/3557]: rust-lang/cargo#3557
[cargo/3604]: rust-lang/cargo#3604
[RFC 1623]: https://github.com/rust-lang/rfcs/blob/master/text/1623-static.md
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 25, 2019
rustc: Flag metadata compatible with multiple CGUs
It looks like the `OutputType::Metadata` kind in the compiler was
misclassified in rust-lang#38571 long ago by accident as incompatible with
codegen units and a single output file. This means that if you emit both
a linkable artifact and metadata it silently turns off multiple codegen
units unintentionally!

This commit corrects the situation to ensure that if `--emit metadata`
is used it doesn't implicitly disable multiple codegen units. This will
ensure we don't accidentally regress compiler performance when striving
to implement pipelined compilation!
Centril added a commit to Centril/rust that referenced this pull request Apr 28, 2019
Rollup merge of rust-lang#60270 - alexcrichton:metadata-multi-cgu, r=…
…oli-obk

rustc: Flag metadata compatible with multiple CGUs

It looks like the `OutputType::Metadata` kind in the compiler was
misclassified in rust-lang#38571 long ago by accident as incompatible with
codegen units and a single output file. This means that if you emit both
a linkable artifact and metadata it silently turns off multiple codegen
units unintentionally!

This commit corrects the situation to ensure that if `--emit metadata`
is used it doesn't implicitly disable multiple codegen units. This will
ensure we don't accidentally regress compiler performance when striving
to implement pipelined compilation!
lzutao added a commit to lzutao/rustc that referenced this pull request Apr 29, 2019
rustc: Flag metadata compatible with multiple CGUs
It looks like the `OutputType::Metadata` kind in the compiler was
misclassified in rust-lang#38571 long ago by accident as incompatible with
codegen units and a single output file. This means that if you emit both
a linkable artifact and metadata it silently turns off multiple codegen
units unintentionally!

This commit corrects the situation to ensure that if `--emit metadata`
is used it doesn't implicitly disable multiple codegen units. This will
ensure we don't accidentally regress compiler performance when striving
to implement pipelined compilation!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.