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

Reinstate CARGO_PRIMARY_PACKAGE (take 2) #8758

Merged
merged 5 commits into from
Oct 14, 2020

Conversation

ebroto
Copy link
Member

@ebroto ebroto commented Oct 6, 2020

As discussed in this Zulip thread

r? @ehuss

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 6, 2020
@alexcrichton
Copy link
Member

Would you be ok adding documentation for this new environment variable as well as a test?

@ehuss
Copy link
Contributor

ehuss commented Oct 8, 2020

Ignore me.
Just a minor note: It might also be worthwhile to consider a more precise name. This is only set for the root units, and isn't necessarily the root "package". For example, if you run cargo test, any binaries in a package will be built, but they aren't "primary". Or cargo build --bin foo, the binary target will be a root, but the library will not be marked as "primary".

Maybe CARGO_ROOT_TARGET? CARGO_ROOT_UNIT? CARGO_PRIMARY_TARGET? CARGO_ROOT_CRATE? Hm, these terms are a bit overloaded, vague, or potentially confusing. Any other ideas?

@ehuss
Copy link
Contributor

ehuss commented Oct 8, 2020

Oh, wait, nevermind, I misread how that is implemented. CARGO_PRIMARY_PACKAGE is good I think.

@ebroto
Copy link
Member Author

ebroto commented Oct 12, 2020

Would you be ok adding documentation for this new environment variable as well as a test?

Of course! Besides the doc, I've added the following tests:

  • in build.rs, make sure it's only set for the primary package and not for deps of any kind
  • in workspaces.rs, one for "simple" and one for virtual workspaces, trying some combinations of -p and --all.

Please tell me if I missed some cases or if I put them in the wrong place :)

@ehuss ehuss added the T-cargo Team: Cargo label Oct 12, 2020
@ehuss
Copy link
Contributor

ehuss commented Oct 12, 2020

Thanks!

@rust-lang/cargo This will add a new, stable environment variable set when running rustc. CARGO_PRIMARY_PACKAGE indicates that this is one of the "root" packages requested on the command-line (via -p or the default package). The use case for this is primarily for clippy, so that it can know if it should emit lints. In particular, this will help with the --fix flag, so that it doesn't attempt to run clippy on other dependencies in a workspace, which can cause errors and problems. This can also help with solving rust-lang/rust-clippy#3025, though that issue will require some additional UI on the clippy side to determine whether or not it runs for all members. This should help lead towards stabilizing #8143 (RUSTC_WORKSPACE_WRAPPER) which will fix the caching problems with clippy.

I think it is OK to go straight to stable for this, though feel free to disagree. I expect that after updating clippy and testing some things out, we can move to stabilize RUSTC_WORKSPACE_WRAPPER soon as well.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 12, 2020

Team member @ehuss 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 An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Oct 12, 2020
@Mark-Simulacrum
Copy link
Member

FWIW perf.rlo is also interested in this and uses a hack (cargo rustc -- --wrap-rustc-with $prog) to workaround this today; the extra argument is intercepted with a wrapper script and stripped (some other processing is also done). Once this is added I'd definitely be interested in reworking perf to rely on it over the wrap-rustc-with option.

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 13, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Oct 13, 2020
@ehuss
Copy link
Contributor

ehuss commented Oct 14, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 14, 2020

📌 Commit 6a94a3b has been approved by ehuss

@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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2020
@bors
Copy link
Contributor

bors commented Oct 14, 2020

⌛ Testing commit 6a94a3b with merge 900f981...

bors added a commit that referenced this pull request Oct 14, 2020
@bors
Copy link
Contributor

bors commented Oct 14, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 14, 2020
@ehuss
Copy link
Contributor

ehuss commented Oct 14, 2020

Oh, I should have paid closer attention to the CI status. @ebroto, the tests can't check the environment variables via the verbose output, since the output is different for different platforms. Can you update the tests to a different approach? There are a variety of different options. Maybe just use env! to capture the value, and use cargo run or cargo test to run some code the verify the value is correct?

Environment variables are represented differently in differents OSes in
the output.

Add tests checking if the variable is set instead.
@ebroto
Copy link
Member Author

ebroto commented Oct 14, 2020

@ehuss I've reworked the tests following your suggestion. I think it helped to simplify the implementation. Thanks!

@ehuss
Copy link
Contributor

ehuss commented Oct 14, 2020

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Oct 14, 2020

📌 Commit fb02ade has been approved by ehuss

@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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2020
@bors
Copy link
Contributor

bors commented Oct 14, 2020

⌛ Testing commit fb02ade with merge 12db56c...

@bors
Copy link
Contributor

bors commented Oct 14, 2020

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 12db56c to master...

@bors bors merged commit 12db56c into rust-lang:master Oct 14, 2020
@ebroto ebroto deleted the cargo_primary_package branch October 14, 2020 23:38
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2020
Update cargo

11 commits in 9d1a4863abd9237dbf9d1b74c78632b6a205f6bb..12db56cdedbc2c26a9aa18f994c0188cdcc67df5
2020-10-05 18:29:52 +0000 to 2020-10-14 23:07:45 +0000
- Reinstate CARGO_PRIMARY_PACKAGE (take 2) (rust-lang/cargo#8758)
- Add actionable help message for --features (rust-lang/cargo#8773)
- Fix panic in `cargo doc` with -Zfeatures=itarget (rust-lang/cargo#8777)
- Update git2. (rust-lang/cargo#8778)
- Document RUSTFMT environment variable (rust-lang/cargo#8767)
- Update crossbeam-utils requirement from 0.7 to 0.8 (rust-lang/cargo#8769)
- Update toml dependency (rust-lang/cargo#8772)
- Mark proc-macro crates (rust-lang/cargo#8765)
- cargo-tree: mention special target `all` in CLI help text (rust-lang/cargo#8766)
- Bump to 0.50.0, update changelog (rust-lang/cargo#8764)
- Update deprecated GitHub add-path in workflows. (rust-lang/cargo#8760)
bors added a commit to rust-lang/rust-clippy that referenced this pull request Oct 22, 2020
Add --no-deps option to avoid running on path dependencies in workspaces

Since rust-lang/cargo#8758 has hit nightly, this allows us to address the second bullet point and [the concern related to `--fix`](rust-lang/cargo#8143 (comment)) in the [RUSTC_WORKSPACE_WRAPPER tracking issue](rust-lang/cargo#8143).

As a reminder stabilizing that env var will solve #4612 (Clippy not running after `cargo check` in stable) and would allow to stabilize the `--fix` option in Clippy.

changelog: Add `--no-deps` option to avoid running on path dependencies in workspaces

Fixes #3025
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Oct 23, 2020
bors added a commit to rust-lang/rust-clippy that referenced this pull request Dec 9, 2020
Add --no-deps option to avoid running on path dependencies in workspaces

Since rust-lang/cargo#8758 has hit nightly, this allows us to address the second bullet point and [the concern related to `--fix`](rust-lang/cargo#8143 (comment)) in the [RUSTC_WORKSPACE_WRAPPER tracking issue](rust-lang/cargo#8143).

As a reminder stabilizing that env var will solve #4612 (Clippy not running after `cargo check` in stable) and would allow to stabilize the `--fix` option in Clippy.

changelog: Add `--no-deps` option to avoid running on path dependencies in workspaces

Fixes #3025
fpoli added a commit to viperproject/prusti-dev that referenced this pull request Dec 10, 2020
fpoli added a commit to viperproject/prusti-dev that referenced this pull request Dec 14, 2020
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 14, 2021
Pkgsrc changes:
 * Adjust patches, convert tabs to spaces so that tests pass.
 * Remove patches which are no longer needed (upstream changed)
 * Minor adjustments for SunOS, e.g. disable stack probes.
 * Adjust cargo checksum patching accordingly.
 * Remove commented-out use of PATCHELF on NetBSD, which doesn't work anyway...

Upstream changes:

Version 1.49.0 (2020-12-31)
============================

Language
-----------------------

- [Unions can now implement `Drop`, and you can now have a field in a union
  with `ManuallyDrop<T>`.][77547]
- [You can now cast uninhabited enums to integers.][76199]
- [You can now bind by reference and by move in patterns.][76119] This
  allows you to selectively borrow individual components of a type. E.g.
  ```rust
  #[derive(Debug)]
  struct Person {
      name: String,
      age: u8,
  }

  let person = Person {
      name: String::from("Alice"),
      age: 20,
  };

  // `name` is moved out of person, but `age` is referenced.
  let Person { name, ref age } = person;
  println!("{} {}", name, age);
  ```

Compiler
-----------------------

- [Added tier 1\* support for `aarch64-unknown-linux-gnu`.][78228]
- [Added tier 2 support for `aarch64-apple-darwin`.][75991]
- [Added tier 2 support for `aarch64-pc-windows-msvc`.][75914]
- [Added tier 3 support for `mipsel-unknown-none`.][78676]
- [Raised the minimum supported LLVM version to LLVM 9.][78848]
- [Output from threads spawned in tests is now captured.][78227]
- [Change os and vendor values to "none" and "unknown" for some targets][78951]

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

Libraries
-----------------------

- [`RangeInclusive` now checks for exhaustion when calling `contains`
  and indexing.][78109]
- [`ToString::to_string` now no longer shrinks the internal buffer
  in the default implementation.][77997]
- [`ops::{Index, IndexMut}` are now implemented for fixed sized
  arrays of any length.][74989]

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

- [`slice::select_nth_unstable`]
- [`slice::select_nth_unstable_by`]
- [`slice::select_nth_unstable_by_key`]

The following previously stable methods are now `const`.

- [`Poll::is_ready`]
- [`Poll::is_pending`]

Cargo
-----------------------
- [Building a crate with `cargo-package` should now be independently
  reproducible.][cargo/8864]
- [`cargo-tree` now marks proc-macro crates.][cargo/8765]
- [Added `CARGO_PRIMARY_PACKAGE` build-time environment
  variable.]  [cargo/8758] This variable will be set if the crate
  being built is one the user selected to build, either with `-p`
  or through defaults.
- [You can now use glob patterns when specifying packages &
  targets.][cargo/8752]


Compatibility Notes
-------------------
- [Demoted `i686-unknown-freebsd` from host tier 2 to target tier
  2 support.][78746]
- [Macros that end with a semi-colon are now treated as statements
  even if they expand to nothing.][78376]
- [Rustc will now check for the validity of some built-in attributes
  on enum variants.][77015] Previously such invalid or unused
  attributes could be ignored.
- Leading whitespace is stripped more uniformly in documentation
  comments, which may change behavior. You read [this post about
  the changes][rustdoc-ws-post] for more details.
- [Trait bounds are no longer inferred for associated types.][79904]

Internal Only
-------------
These changes provide no direct user facing benefits, but represent
significant improvements to the internals and overall performance
of rustc and related tools.

- [rustc's internal crates are now compiled using the `initial-exec` Thread
  Local Storage model.][78201]
- [Calculate visibilities once in resolve.][78077]
- [Added `system` to the `llvm-libunwind` bootstrap config option.][77703]
- [Added `--color` for configuring terminal color support to bootstrap.][79004]


[75991]: rust-lang/rust#75991
[78951]: rust-lang/rust#78951
[78848]: rust-lang/rust#78848
[78746]: rust-lang/rust#78746
[78376]: rust-lang/rust#78376
[78228]: rust-lang/rust#78228
[78227]: rust-lang/rust#78227
[78201]: rust-lang/rust#78201
[78109]: rust-lang/rust#78109
[78077]: rust-lang/rust#78077
[77997]: rust-lang/rust#77997
[77703]: rust-lang/rust#77703
[77547]: rust-lang/rust#77547
[77015]: rust-lang/rust#77015
[76199]: rust-lang/rust#76199
[76119]: rust-lang/rust#76119
[75914]: rust-lang/rust#75914
[74989]: rust-lang/rust#74989
[79004]: rust-lang/rust#79004
[78676]: rust-lang/rust#78676
[79904]: rust-lang/rust#79904
[cargo/8864]: rust-lang/cargo#8864
[cargo/8765]: rust-lang/cargo#8765
[cargo/8758]: rust-lang/cargo#8758
[cargo/8752]: rust-lang/cargo#8752
[`slice::select_nth_unstable`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable
[`slice::select_nth_unstable_by`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by
[`slice::select_nth_unstable_by_key`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by_key
[`hint::spin_loop`]: https://doc.rust-lang.org/stable/std/hint/fn.spin_loop.html
[`Poll::is_ready`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_ready
[`Poll::is_pending`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_pending
[rustdoc-ws-post]: https://blog.guillaume-gomez.fr/articles/2020-11-11+New+doc+comment+handling+in+rustdoc
@ehuss ehuss added this to the 1.49.0 milestone Feb 6, 2022
@ghost ghost mentioned this pull request May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants