Skip to content

Canonicalize default target if it ends with .json #6778

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 25, 2019
Merged

Canonicalize default target if it ends with .json #6778

merged 1 commit into from
Mar 25, 2019

Conversation

phil-opp
Copy link
Contributor

@phil-opp phil-opp commented Mar 23, 2019 •
edited
Loading

Targets that end with .json are not target triples but paths to target configuration files. We currently canonicalize all .json paths that are passed as --target so that the paths are still valid when building dependencies (where the current working directory is changed).

This commit adds the same canonicalization to default targets specified in a build.target key in a .cargo/config file by adding a new Config::target_triple function.

@rust-highfive
Copy link

r? @alexcrichton

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2019
@phil-opp phil-opp mentioned this pull request Mar 23, 2019
Merged
2 tasks
@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Contributor

bors commented Mar 25, 2019

📌 Commit 7196d6944c22e77d9825286202cf52287f3299e8 has been approved by alexcrichton

@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 Mar 25, 2019
@bors
Copy link
Contributor

bors commented Mar 25, 2019

⌛ Testing commit 7196d6944c22e77d9825286202cf52287f3299e8 with merge e80c47a3376345cd55a3dc612f3403c62d3b79ad...

@phil-opp
Copy link
Contributor Author

phil-opp commented Mar 25, 2019 •
edited
Loading

@alexcrichton I just noticed that this solution has a problem with building from subdirectories because it does not create paths relative to the config file. I have a new solution without that problem almost ready. Could you abort the current build? Sorry for not telling you earlier, I wasn't sure whether the new approach would work!

@phil-opp phil-opp force-pushed the canonicalize-config-target branch from 7196d69 to 3de1f51 Compare March 25, 2019 14:29
@alexcrichton
Copy link
Member

@bors: r-

No worries!

@phil-opp phil-opp force-pushed the canonicalize-config-target branch from 3de1f51 to 0f7f152 Compare March 25, 2019 14:46
@phil-opp
Copy link
Contributor Author

phil-opp commented Mar 25, 2019 •
edited
Loading

Thanks! I just pushed the new version. It creates a new Config::target_triple function that canonicalizes targets ending in .json relative to their definition. I verified locally that builds from subdirectories now work too. Edit: Seems like there are still some problems. Please do not merge yet.

@phil-opp phil-opp closed this Mar 25, 2019
@phil-opp
Copy link
Contributor Author

Seems like there are still some problems. Please do not merge yet.

It turns out that cargo-xbuild was at fault: it did not respect the CARGO environment variable so it built the sysroot using the default, unpatched cargo. I fixed these errors in cargo-xbuild and now this patch works reproducibly without problems.

So this should be ready for review now, @alexcrichton. Sorry for the confusion!

@phil-opp phil-opp reopened this Mar 25, 2019
@alexcrichton
Copy link
Member

Having the duplication in logic here is somewhat unfortunate because ideally we'd just have the ".json" suffix check in the same function. I wonder if we can do that and also avoid canonicalization for config paths? We already have Config::get_path which is like get_string, but creates an absolute path for us already. If the get_string value ends in json, could this fall back to executing get_path and using that?

@phil-opp phil-opp force-pushed the canonicalize-config-target branch 2 times, most recently from 64ccbe8 to 9475ea6 Compare March 25, 2019 18:48
@phil-opp phil-opp force-pushed the canonicalize-config-target branch from 9475ea6 to 703f7a7 Compare March 25, 2019 18:58
@phil-opp
Copy link
Contributor Author

You're right, it seems like the config path is already canonicalized. I pushed a new version that avoids the duplication.

Instead of get_path I join the definition and the value manually to avoid a double lookup. The only remaining duplication is for converting a Path to a String with error checking.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 25, 2019

📌 Commit 703f7a7 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Mar 25, 2019

⌛ Testing commit 703f7a7 with merge 4cc4337...

bors added a commit that referenced this pull request Mar 25, 2019
@bors
Auto merge of #6778 - phil-opp:canonicalize-config-target, r=alexcric…
4cc4337 
…hton

Canonicalize default target if it ends with `.json`

Targets that end with `.json` are not target triples but paths to target configuration files. We currently canonicalize all `.json` paths that are passed as `--target` so that the paths are still valid when building dependencies (where the current working directory is changed).

This commit adds the same canonicalization to default targets specified in a `build.target` key in a `.cargo/config` file by adding a new `Config::target_triple` function.
@bors
Copy link
Contributor

bors commented Mar 25, 2019

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

@bors bors merged commit 703f7a7 into rust-lang:master Mar 25, 2019
@phil-opp phil-opp deleted the canonicalize-config-target branch March 25, 2019 20:47
@ehuss ehuss mentioned this pull request Mar 29, 2019
Merged
bors added a commit to rust-lang/rust that referenced this pull request Mar 30, 2019
@bors
Auto merge of #59516 - ehuss:update-cargo, r=alexcrichton
befeeb7 
Update cargo

Update cargo

22 commits in 0e35bd8af0ec72d3225c4819b330b94628f0e9d0..63231f438a2b5b84ccf319a5de22343ee0316323
2019-03-13 06:52:51 +0000 to 2019-03-27 12:26:45 +0000
- Code cleanup (rust-lang/cargo#6787)
- Add cargo:rustc-link-arg to pass custom linker arguments (rust-lang/cargo#6298)
- Testsuite: remove some unnecessary is_nightly checks. (rust-lang/cargo#6786)
- cargo metadata: Don't show `null` deps. (rust-lang/cargo#6534)
- Some fingerprint cleanup. (rust-lang/cargo#6785)
- Fix fingerprint for canceled build script. (rust-lang/cargo#6782)
- Canonicalize default target if it ends with `.json` (rust-lang/cargo#6778)
- Fix setting `panic=unwind` compiling lib a extra time. (rust-lang/cargo#6781)
- Always nicely show errors from crates.io if possible (rust-lang/cargo#6771)
- Testsuite: Make `cwd()` relative to project root. (rust-lang/cargo#6768)
- Allow `cargo fix` if gitignore matches root working dir. (rust-lang/cargo#6767)
- Remove redundant imports (rust-lang/cargo#6763)
- Handle backcompat hazard with `toml` crate (rust-lang/cargo#6761)
- Fix spurious error in dirty_both_lib_and_test. (rust-lang/cargo#6756)
- Update toml requirement from 0.4.2 to 0.5.0 (rust-lang/cargo#6760)
- Reuse std::env::consts::EXE_SUFFIX (rust-lang/cargo#6758)
- Proptest 0.9.1 (rust-lang/cargo#6753)
- Don't need extern crate in 2018 (rust-lang/cargo#6752)
- Release a jobserver token while locking a file (rust-lang/cargo#6748)
- Minor doc fix for publish command synopsis (rust-lang/cargo#6749)
- Stricter package change detection. (rust-lang/cargo#6740)
- Fix resolving yanked crates when using a local registry. (rust-lang/cargo#6742)
bors bot added a commit to rust-osdev/bootloader that referenced this pull request Apr 1, 2019
@bors @phil-opp
Merge #51
e1c6330 
51: Rewrite build system r=phil-opp a=phil-opp

- [x] Blocked on rust-lang/rust#59351
- [x] Blocked on rust-lang/cargo#6778

Fixes #33 
Fixes #48

Co-authored-by: Philipp Oppermann <dev@phil-opp.com>
@phil-opp phil-opp mentioned this pull request Apr 11, 2020
Open
@ehuss ehuss added this to the 1.35.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.35.0
Development

Successfully merging this pull request may close these issues.
5 participants
@phil-opp @rust-highfive @alexcrichton @bors @ehuss