-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Switching cwd breaks usage of custom target specifications #4905
Comments
We could use absolute paths for target triples, i.e. Unfortunately, absolute paths don't work today, since the target triple path is just pushed to the sysroot path: // in src/librustc/session/filesearch.rs
pub fn relative_target_lib_path(sysroot: &Path, target_triple: &str) -> PathBuf {
let mut p = PathBuf::from(find_libdir(sysroot).as_ref());
assert!(p.is_relative());
p.push(RUST_LIB_DIR);
p.push(target_triple); <-- problem if the path is absolute
p.push("lib");
p
} If |
@alexcrichton @carols10cents @matklad This issue is affecting lots of people (everyone who uses custom targets) and has not received any attention yet. So I thought I'd ping you as members of cargo team, maybe you could take a look. |
@phil-opp I think it's unlikely we'll be reverting to the previous behavior, at this point I think it's best to consider how to change custom targets such that they work with Cargo "by default", although I'm not quite sure what that would mean. |
One option for Cargo is to automatically pass an absolute path for anything it detects as JSON (aka ends with json and a file exists in the cwd), and then we could fix rustc's logic to work with absolute paths. |
@alexcrichton Why not do something similar, but limit it to a file named $target.json? I believe this would restore the previous functionality. |
@SergioBenitez yep that'd also work! |
@alexcrichton Thanks for the quick reply and showing a path forward! |
Seems like the target triple is stored in the metadata of This means that we already have to compile the sysroot for the absolute target and can't just convert the triple to an absolute path in |
The PR triple is ready: rust-lang/rust#49019, #5228, and japaric/xargo#205. The idea is to add special treatment for target arguments ending in The three PRs introduce support for absolute target paths, which makes setting In summary, these PRs allow |
Add support for absolute target.json paths Builds upon rust-lang/rust#49019 with the goal to provide a solution to #4905. This PR does two things: ~~1. It appends a hash of the target path to the target folder name if a `*.json` path is passed as `--target`, like it's done in rust-lang/rust#49019. This helps differentiating targets with the same JSON file name and avoids sysroot clashes in `xargo`.~~ See #5228 (comment) 2. It canonicalizes the passed target path (if it's a `*.json` path), so that the path stays valid when building dependencies and setting the `RUST_TARGET_PATH` environment variable is no longer necessary.
Introduce a TargetTriple enum to support absolute target paths This PR replaces target triple strings with a `TargetTriple` enum, which represents either a target triple or a path to a JSON target file. The path variant is used if the `--target` argument has a `.json` extension, else the target triple variant is used. The motivation of this PR is support for absolute target paths to avoid the need for setting the `RUST_TARGET_PATH` environment variable (see rust-lang/cargo#4905 for more information). For places where some kind of triple is needed (e.g. in the sysroot folder), we use the file name (without extension). For compatibility, we keep the old behavior of searching for a file named `$(target_triple).json` in `RUST_TARGET_PATH` for non-official target triples.
With rust-lang/rust#49019 and #5228 merged, the example from the issue description "works" now if It still doesn't work with |
A natural extension of this (maybe belongs in a different bug), is to support distributing targets in cargo packages. This would enable re-usability, as well as bundling a target with a platform-specific library. |
Per japaric/xargo#186 (comment), the following no longer works:
The following conversation was taken from #4865:
@SergioBenitez
@alexcrichton
Workaround
In order to fix the cargo build error, you need to manually set
RUST_TARGET_PATH
.Remaining Discussion
I think the following question remains:
Why are users of custom target specifications required to set
RUST_TARGET_PATH
every timecargo build
is invoked?The error message does not make it clear why the build is failing. Also, it is unintuitive to set an environment variable pointing to the current directory for the build to succeed.
If the current behavior will remain in the long-term, there should probably be documentation somewhere that addresses the breaking change. Also, I think the Cargo error message should hint that
RUST_TARGET_PATH
is pointing to the wrong directory.The text was updated successfully, but these errors were encountered: