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

Support absolute target JSON paths #205

Closed
wants to merge 1 commit into from

Conversation

phil-opp
Copy link

@phil-opp phil-opp commented Mar 22, 2018

Builds upon rust-lang/rust#49019 and rust-lang/cargo#5228 to provide a solution to rust-lang/cargo#4905 and #194.

The idea is that we allow targets ending in *.json (they were forbidden before) and convert them to canonicalized paths. Thus, they just work when compiling dependencies without the need to set the RUST_TARGET_PATH environment variable.

This PR just removes the explicit prohibition of paths as target and extends Target::new to handle such target paths. This should not lead to any breakage, because target paths ending in .json were explicitly forbidden before.

@japaric
Copy link
Owner

japaric commented Mar 26, 2018

cargo build --target ../foo.json doesn't work when using Cargo due to rust-lang/cargo#4905 so I'd rather not make that special case work for Xargo. In general, I don't want Xargo to provide features that Cargo doesn't other than just building the sysroot because (a) that makes it harder to land Xargo's functionality in Cargo (should these extra features land as well?), and because (b) that could make people stick to Xargo when Cargo gains Xargo's functionality.

@phil-opp
Copy link
Author

phil-opp commented Mar 26, 2018

I just realized that the PR description is a bit high level. Sorry about that! Let me explain in more detail:

cargo build --target ../foo.json doesn't work when using Cargo due to rust-lang/cargo#4905 so I'd rather not make that special case work for Xargo.

I'm trying to fix rust-lang/cargo#4905 and already got a cargo PR for this merged. Target paths ending in .json work now from the cargo site and we even have a test case for --target src/../custom-target.json.

There is one remaining issue in rustc (see rust-lang/cargo#4905 (comment)), which will be fixed by rust-lang/rust#49019. Afterwards, rustc and cargo will support target paths (ending in .json) and even convert them to absolute paths automatically, so that cargo build --target foo.json will work without RUST_TARGET_PATH.

However, when we try xargo with the new rustc/cargo versions, two issues arise:

  1. xargo does an extra check to forbid targets ending in .json.
  2. xargo does not pass the original --target argument when building the sysroot, but only the triple without extension. Thus, the sysroot metadata does not match the target that is later passed to cargo and an error occurs, since the core library was compiled for target "foo" and not "foo.json".

I pushed a new version that makes it clearer that only the above two things are fixed.

@phil-opp
Copy link
Author

rust-lang/rust#49019 was just merged and will be in the next nightly.

@phil-opp
Copy link
Author

phil-opp commented Apr 2, 2018

The current nightly contains all necessary rustc and cargo changes, so this PR can now be tested via:

cargo install --git https://github.com/phil-opp/xargo.git --branch target-spec --force

Then xargo build --target foo.json (and also --target src/../foo.json) should work.

@phil-opp
Copy link
Author

phil-opp commented Apr 7, 2018

@japaric Did you have some time to look at this? This PR is the only missing piece to allow compilation without RUST_TARGET_PATH again. It does not add any new features to xargo, it only removes the explicit prohibition of targets ending in .json and no longer modifies the target name that is passed to cargo (see #205 (comment) for details).

@phil-opp
Copy link
Author

Given that this PR has been open for over a year now without any progress, I'm going to give it a close. For what it's worth, we use this patch in cargo-xbuild without problems.

@phil-opp phil-opp closed this Jul 17, 2019
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.

2 participants