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

Add new packages to [workspace.members] automatically #12779

Merged
merged 3 commits into from
Oct 29, 2023

Conversation

calavera
Copy link
Contributor

@calavera calavera commented Oct 6, 2023

What does this PR try to resolve?

If a new package is created in a workspace, this change adds the package's path to the workspace's members list automatically.

It doesn't add the package to the list if the path is in the workspace's exclude list, or if the members list doesn't exist already. I noticed that a cargo_new test broke if I create the members list when it doesn't exist. This is because the workspace's manifest can be used for package templating. I think it's better to not break that use case.

Fixes #6378

How should we test and review this PR?

I've included tests in the cargo_new suite.

src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented Oct 7, 2023

r? epage

@calavera calavera force-pushed the workspace_adds_implicit_members branch from 820cdc4 to dcd0c49 Compare October 9, 2023 18:19
@calavera
Copy link
Contributor Author

calavera commented Oct 9, 2023

@epage it looks like there are some formatting incompatibilities when running the tests with Windows MSVC stable. Is there a way to insert the data correctly regardless of the version? See this example:

   4    4 |     "crates/bar",
   5      -     "crates/foo",
        5 +     'crates/foo',
   6    6 | ]

https://github.com/rust-lang/cargo/actions/runs/6460323327/job/17537796417?pr=12779#step:10:3698

Is there a reason why toml_edit uses single quotes on Windows? I cannot see any way to change that behavior.

@calavera
Copy link
Contributor Author

calavera commented Oct 10, 2023

I've also noticed that this comparison fails on Windows, I'm guessing because of encoding issues. When I try to debug it, the values seem to be displayed correctly, but the comparison fails:

let pat = member.as_str().unwrap();
if pat == relpath {
    return Ok(());
}
println!("{} - {} EQUAL {}", pat, relpath, pat == relpath);

crates/foo - crates/foo EQUAL false

Is there a way to do this consistently across platforms?

@epage
Copy link
Contributor

epage commented Oct 10, 2023

Could you show the debug representation for each?

@calavera
Copy link
Contributor Author

Could you show the debug representation for each?

oh, thanks for the hint! it looks like the relative path returned from pathdiff has two slashes on Windows:

"crates/foo" - "crates//foo" EQUAL false

I've changed the implementation to compare Path elements, and fix the path separator differences when the path is serialized in the Cargo.toml file.

@calavera
Copy link
Contributor Author

I've added a test for cargo init too.

@epage let me know what you think about these changes.

@epage
Copy link
Contributor

epage commented Oct 11, 2023

At this point, it comes down to what we want out policy to be for formatting.

@calavera
Copy link
Contributor Author

At this point, it comes down to what we want out policy to be for formatting.

@joshtriplett's suggestion makes sense to me: https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Automatic.20edits.20to.20.60Cargo.2Ctoml.60/near/395834270

What this means in this case:

  1. If there is already a members list, the format of the list is preserved.
  2. If there is not a members list in the manfest already, the format doesn't break anything. The list is always added as the last element to the workspace elements list. The format for this last element follows the same format as the previous last element.

@epage
Copy link
Contributor

epage commented Oct 13, 2023

Let's go ahead and remove the auto-formatting but keep the relevant tests. If nothing else, this is an improvement over what we already have and changing it isn't a breaking change.

@calavera
Copy link
Contributor Author

@epage I've cleaned up the auto-formatting logic. The only formatting left is something that would really bother me as a user if we didn't do, which is adding a new line between the members element and any inline table below.

This are two relevant tests for that formatting:

Let me know what you think.

src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_new.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Oct 28, 2023

Everything at this point is minor. Feel free to clean up the commits.

@calavera calavera force-pushed the workspace_adds_implicit_members branch from 41aa44b to 0f03d77 Compare October 28, 2023 14:37
@calavera
Copy link
Contributor Author

@epage I've reworked the commits to make the changes more isolated. I put dependency changes in separated commits, and the feature implementation itself in one commit. I don't think the changes in cargo_new are big enough to grant a more fine grained separation because everything is encapsulated in a new method.

Let me know what you think.

This function can be used by other commands to figure out whether a list is sorted or not.

Signed-off-by: David Calavera <[email protected]>
This new version incorporates utilities to sort Array elements.

Signed-off-by: David Calavera <[email protected]>
When a user runs `cargo new` or `cargo init` within a workspace, Cargo will automatically add the new package to the members list in the workspace if necessary. The heuristic to add the new package is as follows:

- If there is no `members` list in the workspace yet, a new `members` list is created.
- If there is an `exclude` statement, Cargo checks if the new package should be excluded. If it doesn't match the `exclude` list, the package is added to the `members` list.
- If there is a glob expression in the `members` list that matches the new package, the package is not added to the `members` list.
- If the existent `members` list is sorted, Cargo tries to preserve the ordering when it adds the new package.

This change doesn't try to format the resulting `members` list in any way, leaving the formatting decissions to the user.

Signed-off-by: David Calavera <[email protected]>
@calavera calavera force-pushed the workspace_adds_implicit_members branch from 0f03d77 to 1a8bfdf Compare October 28, 2023 14:42
@epage
Copy link
Contributor

epage commented Oct 29, 2023

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 29, 2023

📌 Commit 1a8bfdf has been approved by epage

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 29, 2023
@bors
Copy link
Contributor

bors commented Oct 29, 2023

⌛ Testing commit 1a8bfdf with merge 04621e2...

@bors
Copy link
Contributor

bors commented Oct 29, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 04621e2 to master...

@bors bors merged commit 04621e2 into rust-lang:master Oct 29, 2023
22 checks passed
@calavera calavera deleted the workspace_adds_implicit_members branch October 29, 2023 16:24
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 31, 2023
Update cargo

7 commits in 708383d620e183a9ece69b8fe930c411d83dee27..b4d18d4bd3db6d872892f6c87c51a02999b80802
2023-10-27 21:09:26 +0000 to 2023-10-31 18:19:10 +0000
- refactor(toml): Cleanup noticed on the way to rust-lang/cargo#12801 (rust-lang/cargo#12902)
- feat(trim-paths): set env `CARGO_TRIM_PATHS` for build scripts (rust-lang/cargo#12900)
- feat: implement RFC 3127 `-Ztrim-paths` (rust-lang/cargo#12625)
- docs: clarify config to use vendored source is printed to stdout (rust-lang/cargo#12893)
- Improve the margin calculation for the search command's UI (rust-lang/cargo#12890)
- Add new packages to [workspace.members] automatically (rust-lang/cargo#12779)
- refactor(toml): Decouple parsing from interning system (rust-lang/cargo#12881)

r? ghost
@ehuss ehuss added this to the 1.75.0 milestone Nov 6, 2023
zhassan-aws added a commit to model-checking/kani that referenced this pull request Nov 6, 2023
Update to the latest Rust toolchain (2023-11-06).

The relevant changes are:
- rust-lang/rust#117507: this required changing
the import of `Span` from `rustc_span::source_map::Span` to
`rustc_span::Span`.
- rust-lang/rust#114208: this changed the data
field for the `OffsetOf` variant of `NullOp` from `List<FieldIdx>` to
`List<(VariantIdx, FieldIdx)>`, which required updating the relevant
code in `rvalue.rs`.
- rust-lang/rust#115626: the unchecked shift
operators have been separated from the `unchecked_math` feature, so this
required changing the feature annotation in
`tests/ui/should-panic-attribute/unexpected-failures/test.rs`
- Some rustc change (not sure which one) result in a line in
`tests/coverage/unreachable/variant/main.rs` getting optimized out. To
maintain what this test is testing, I changed the `match` to make it a
bit less-prone to optimization.
- A change in `cargo` (rust-lang/cargo#12779)
resulted in an update to Kani's workspace `Cargo.toml` when `cargo add`
is executed inside `tests/script-based-pre/build-cache-bin`. This is
apparently intended behavior, so I had to make the `exclude` in the
`Cargo.toml` more specific to make sure this doesn't happen (I tried
using a glob, but that didn't work, apparently because of
rust-lang/cargo#6009.

Resolves #2848 

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 3, 2024
Pkgsrc changes:
 * Adjust patches and cargo checksums to new versions.
 * For an external LLVM, set dependency of llvm >= 16, in accordance
   with the upstream changes.
 * Mark that on NetBSD we now need >= 9.0, so 8.x is no longer supported.
 * On NetBSD/sparc64 10.x, we now need GCC 12 to build the embedded
   LLVM, which is version 17; apparently GCC 10.4 or 10.5 mis-compiles it,
   resulting in an illegal instruction fault during the build.
   Ref. rust-lang/rust#117231

Upstream changes:

Version 1.75.0 (2023-12-28)
==========================

- [Stabilize `async fn` and return-position `impl Trait` in traits.]
  (rust-lang/rust#115822)
- [Allow function pointer signatures containing `&mut T` in `const` contexts.]
  (rust-lang/rust#116015)
- [Match `usize`/`isize` exhaustively with half-open ranges.]
  (rust-lang/rust#116692)
- [Guarantee that `char` has the same size and alignment as `u32`.]
  (rust-lang/rust#116894)
- [Document that the null pointer has the 0 address.]
  (rust-lang/rust#116988)
- [Allow partially moved values in `match`.]
  (rust-lang/rust#103208)
- [Add notes about non-compliant FP behavior on 32bit x86 targets.]
  (rust-lang/rust#113053)
- [Stabilize ratified RISC-V target features.]
  (rust-lang/rust#116485)

Compiler
--------

- [Rework negative coherence to properly consider impls that only
  partly overlap.] (rust-lang/rust#112875)
- [Bump `COINDUCTIVE_OVERLAP_IN_COHERENCE` to deny, and warn in dependencies.]
  (rust-lang/rust#116493)
- [Consider alias bounds when computing liveness in NLL.]
  (rust-lang/rust#116733)
- [Add the V (vector) extension to the `riscv64-linux-android` target spec.]
  (rust-lang/rust#116618)
- [Automatically enable cross-crate inlining for small functions]
  (rust-lang/rust#116505)
- Add several new tier 3 targets:
    - [`csky-unknown-linux-gnuabiv2hf`]
      (rust-lang/rust#117049)
    - [`i586-unknown-netbsd`]
      (rust-lang/rust#117170)
    - [`mipsel-unknown-netbsd`]
      (rust-lang/rust#117356)

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

Libraries
---------

- [Override `Waker::clone_from` to avoid cloning `Waker`s unnecessarily.]
  (rust-lang/rust#96979)
- [Implement `BufRead` for `VecDeque<u8>`.]
  (rust-lang/rust#110604)
- [Implement `FusedIterator` for `DecodeUtf16` when the inner iterator does.]
  (rust-lang/rust#110729)
- [Implement `Not, Bit{And,Or}{,Assign}` for IP addresses.]
  (rust-lang/rust#113747)
- [Implement `Default` for `ExitCode`.]
  (rust-lang/rust#114589)
- [Guarantee representation of None in NPO]
  (rust-lang/rust#115333)
- [Document when atomic loads are guaranteed read-only.]
  (rust-lang/rust#115577)
- [Broaden the consequences of recursive TLS initialization.]
  (rust-lang/rust#116172)
- [Windows: Support sub-millisecond sleep.]
  (rust-lang/rust#116461)
- [Fix generic bound of `str::SplitInclusive`'s `DoubleEndedIterator` impl]
  (rust-lang/rust#100806)
- [Fix exit status / wait status on non-Unix `cfg(unix)` platforms.]
  (rust-lang/rust#115108)

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

- [`Atomic*::from_ptr`]
  (https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicUsize.html#method.from_ptr)
- [`FileTimes`]
  (https://doc.rust-lang.org/stable/std/fs/struct.FileTimes.html)
- [`FileTimesExt`]
  (https://doc.rust-lang.org/stable/std/os/windows/fs/trait.FileTimesExt.html)
- [`File::set_modified`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_modified)
- [`File::set_times`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_times)
- [`IpAddr::to_canonical`]
  (https://doc.rust-lang.org/stable/core/net/enum.IpAddr.html#method.to_canonical)
- [`Ipv6Addr::to_canonical`]
  (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_canonical)
- [`Option::as_slice`]
  (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_slice)
- [`Option::as_mut_slice`]
  (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_mut_slice)
- [`pointer::byte_add`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_add)
- [`pointer::byte_offset`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset)
- [`pointer::byte_offset_from`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset_from)
- [`pointer::byte_sub`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_sub)
- [`pointer::wrapping_byte_add`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_add)
- [`pointer::wrapping_byte_offset`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_offset)
- [`pointer::wrapping_byte_sub`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_sub)

These APIs are now stable in const contexts:

- [`Ipv6Addr::to_ipv4_mapped`]
  (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_ipv4_mapped)
- [`MaybeUninit::assume_init_read`]
  (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.assume_init_read)
- [`MaybeUninit::zeroed`]
  (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.zeroed)
- [`mem::discriminant`]
  (https://doc.rust-lang.org/stable/core/mem/fn.discriminant.html)
- [`mem::zeroed`]
  (https://doc.rust-lang.org/stable/core/mem/fn.zeroed.html)

Cargo
-----

- [Add new packages to `[workspace.members]` automatically.]
  (rust-lang/cargo#12779)
- [Allow version-less `Cargo.toml` manifests.]
  (rust-lang/cargo#12786)
- [Make browser links out of HTML file paths.]
  (rust-lang/cargo#12889)

Rustdoc
-------

- [Accept less invalid Rust in rustdoc.]
  (rust-lang/rust#117450)
- [Document lack of object safety on affected traits.]
  (rust-lang/rust#113241)
- [Hide `#[repr(transparent)]` if it isn't part of the public ABI.]
  (rust-lang/rust#115439)
- [Show enum discriminant if it is a C-like variant.]
  (rust-lang/rust#116142)

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

- [FreeBSD targets now require at least version 12.]
  (rust-lang/rust#114521)
- [Formally demote tier 2 MIPS targets to tier 3.]
  (rust-lang/rust#115238)
- [Make misalignment a hard error in `const` contexts.]
  (rust-lang/rust#115524)
- [Fix detecting references to packed unsized fields.]
  (rust-lang/rust#115583)
- [Remove support for compiler plugins.]
  (rust-lang/rust#116412)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues Command-add Command-new S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: cargo new --in-workspace [directory] [name]
5 participants