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

Implement path-bases (RFC 3529) 1/n: path dep and patch support #14360

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

dpaoliello
Copy link
Contributor

RFC: rust-lang/rfcs#3529
Tracking Issue: #14355

This PR add support in path dependencies and patch table and the workspace built-in path base.

@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2024

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) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2024
@dpaoliello dpaoliello mentioned this pull request Aug 6, 2024
10 tasks
@rustbot rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Aug 8, 2024
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/core/features.rs Outdated Show resolved Hide resolved
tests/testsuite/patch.rs Outdated Show resolved Hide resolved
tests/testsuite/patch.rs Outdated Show resolved Hide resolved
tests/testsuite/path.rs Outdated Show resolved Hide resolved
tests/testsuite/path.rs Outdated Show resolved Hide resolved
tests/testsuite/patch.rs Outdated Show resolved Hide resolved
tests/testsuite/path.rs Outdated Show resolved Hide resolved
tests/testsuite/path.rs Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

What odd cases might we need to test?

  • path = """
  • path = "."

Anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it really make sense to have dedicated tests for these? I'm using PathBuf::join, so I'd expect any unusual value for the path base or the path to be handled by the Standard Library/OS.

Copy link
Contributor

Choose a reason for hiding this comment

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

That depends on how the paths are used after this I've had tests that don't do well in odd cases like this. It depends on if you are dependent on the paths being "clean (prefix checks) and making sure the cleaning comes after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, added a few different tests: ., .., "
Also added a test for self-dependency via a path base.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see self_dependency_using_base which is a good case but not the others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

invalid_path_with_base, parent_dir_with_base and current_dir_with_base

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, that was a typo. I didn't mean path = """ but path = "". I was really confused when you mentioned an "invalid" test.

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

epage commented Aug 12, 2024

Done

Wanting to double check that you intended to not push the mentioned changes.

@dpaoliello
Copy link
Contributor Author

Wanting to double check that you intended to not push the mentioned changes.

Yes, sorry, not sure what the correct etiquette is with this. Usually I reply to comments and resolve conversations -> push updated code -> require reviews.

CHANGELOG.md Outdated Show resolved Hide resolved
tests/testsuite/path.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Aug 12, 2024

Yes, sorry, not sure what the correct etiquette is with this. Usually I reply to comments and resolve conversations -> push updated code -> require reviews.

Not many people remember to "request review" and you never know whether someone will or not (in general, I just assume people won't do it).

In general, its strange to look at a PR with things marked "done" without code pushes, particularly if I'm coming back to the PR several days after the message.

@dpaoliello dpaoliello requested a review from epage August 12, 2024 21:00
@dpaoliello dpaoliello force-pushed the basepath1 branch 3 times, most recently from f6c1ae3 to 8d3fe6a Compare August 12, 2024 23:05
@epage
Copy link
Contributor

epage commented Aug 14, 2024

If "." is handled correctly, "" likely is as well. Going to go ahead with this

@bors r+

@bors
Copy link
Contributor

bors commented Aug 14, 2024

📌 Commit 502c74e has been approved by epage

It is now in the queue for this repository.

@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 Aug 14, 2024
@bors
Copy link
Contributor

bors commented Aug 14, 2024

⌛ Testing commit 502c74e with merge ff6df29...

@bors
Copy link
Contributor

bors commented Aug 14, 2024

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

@bors bors merged commit ff6df29 into rust-lang:master Aug 14, 2024
22 checks passed
@dpaoliello dpaoliello deleted the basepath1 branch August 14, 2024 19:36
@dpaoliello dpaoliello changed the title Implement base paths (RFC 3529) 1/n: path dep and patch support Implement path-bases (RFC 3529) 1/n: path dep and patch support Aug 16, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 17, 2024
Update cargo

8 commits in 2f738d617c6ead388f899802dd1a7fd66858a691..ba8b39413c74d08494f94a7542fe79aa636e1661
2024-08-13 10:57:52 +0000 to 2024-08-16 22:48:57 +0000
- feat(update): Report when incompatible-rust-version packages are selected (rust-lang/cargo#14401)
- test: Migrate old_cargos to snapbox (rust-lang/cargo#14410)
- Correct diagnostic for `TomlDebugInfo` (rust-lang/cargo#14413)
- Add `--lockfile-path` flag (rust-lang/cargo#14326)
- test: Migrate some json tests to snapbox (rust-lang/cargo#14402)
- Implement base paths (RFC 3529) 1/n: path dep and patch support (rust-lang/cargo#14360)
- doc: convert comments to rustdoc in workspace (rust-lang/cargo#14397)
- Fix MSRV for workspace .package and .dependencies (rust-lang/cargo#14400)

r? ghost
@rustbot rustbot added this to the 1.82.0 milestone Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants