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

Only use non-absolute paths for path dependencies #5935

Merged
merged 1 commit into from
Aug 24, 2018

Conversation

alexcrichton
Copy link
Member

Previously Cargo would use a non-absolute path for any dependency contained
within the workspace root but this switches Cargo to only using relative paths
for path dependencies. In practice this shouldn't make much difference, but
for vendored crates and moving around CARGO_HOME it can produce more
consistent results when target directories are shared.

Closes #5923

@rust-highfive
Copy link

r? @matklad

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

@dwijnand
Copy link
Member

dwijnand commented Aug 23, 2018

How is the test asserting the fix in behaviour?

Also, from your wording, is there a difference between non-absolute and relative?

@alexcrichton
Copy link
Member Author

Ah the test is asserting the correct behavior by ensuring that the second compile doesn't print that it's compiling baz, showing that the cached verion was used.

The path passed to rustc largely only affects error messages and debuginfo, but due to debuginfo it affects the final artifact which means Cargo has to cache it separately.

@bors
Copy link
Collaborator

bors commented Aug 24, 2018

☔ The latest upstream changes (presumably #5628) made this pull request unmergeable. Please resolve the merge conflicts.

Previously Cargo would use a non-absolute path for any dependency contained
within the workspace root but this switches Cargo to only using relative paths
for `path` dependencies. In practice this shouldn't make much difference, but
for vendored crates and moving around `CARGO_HOME` it can produce more
consistent results when target directories are shared.

Closes rust-lang#5923
@alexcrichton
Copy link
Member Author

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned matklad Aug 24, 2018
@ehuss
Copy link
Contributor

ehuss commented Aug 24, 2018

@bors r+

I'm not very familiar with vendoring packages, but I can't think of any specific problems with this.

I'll look at the error with deep_dependencies_trigger_rebuild. It has triggered several times lately.

@bors
Copy link
Collaborator

bors commented Aug 24, 2018

📌 Commit 6e57be5 has been approved by ehuss

@bors
Copy link
Collaborator

bors commented Aug 24, 2018

⌛ Testing commit 6e57be5 with merge 676d866...

bors added a commit that referenced this pull request Aug 24, 2018
Only use non-absolute paths for `path` dependencies

Previously Cargo would use a non-absolute path for any dependency contained
within the workspace root but this switches Cargo to only using relative paths
for `path` dependencies. In practice this shouldn't make much difference, but
for vendored crates and moving around `CARGO_HOME` it can produce more
consistent results when target directories are shared.

Closes #5923
@bors
Copy link
Collaborator

bors commented Aug 24, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: ehuss
Pushing 676d866 to master...

@bors bors merged commit 6e57be5 into rust-lang:master Aug 24, 2018
@dwijnand
Copy link
Member

I'll look at the error with deep_dependencies_trigger_rebuild. It has triggered several times lately.

Did you garner anything? I was thinking of opening an issue about it..

@ehuss
Copy link
Contributor

ehuss commented Aug 30, 2018

Did you garner anything? I was thinking of opening an issue about it..

See #5940. To fix deep_dependencies_trigger_rebuild, the calls to File::create and sleep need to be swapped. However, as you can see in the issue, there are many other tests which can fail. My preference would be to add limited hashing to deal with mtime issues to fix all the tests, but Alex mentioned he preferred to change the mtimes during the tests instead, so I was waiting to see what he had in mind.

@dwijnand
Copy link
Member

Oh damn I thought I'd searched #5940.. makes sense it's there.

deep_dependencies_trigger_rebuild has been failing me on Travis CI very often, so I think we should go with the shallow fix for that one. I'll PR that, thanks for the tip.

bors added a commit that referenced this pull request Aug 30, 2018
…uild, r=alexcrichton

Fix path::deep_dependencies_trigger_rebuild often failing in CI

A shallow fix for `path::deep_dependencies_trigger_rebuild` in particular as it's failed my PRs often.

See #5935 (comment), and #5940 for the bigger picture.
@alexcrichton alexcrichton deleted the vendor-paths branch August 30, 2018 19:54
@ehuss ehuss added this to the 1.30.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo varies between absolute/relative path passing vendor dir to rustc
6 participants