Allow fallback on value package.rust-src if XARGO_RUST_SRC doesn't exist #288

jam1garner · 2020-04-12T17:55:16Z

This allows for including a package table in Xargo.toml, which optionally can contain a rust-src string specifying a rust source path in lieu of the existence of the XARGO_RUST_SRC environment variable.

Example Xargo.toml:

[package]
rust-src = "/home/jam/a/dev/rust-skyline/rust-std-skyline/src"

[dependencies.std]
stage = 0

src/rustc.rs

RalfJung · 2020-04-13T07:13:27Z

@jam1garner thanks for the PR!

I have to say however, that this feels rather ad-hoc. Could you talk a bit about why you want this new feature, and why the [package] section is the best one for this?

RalfJung · 2020-04-13T07:43:43Z

The CI failure seems to also occur on master, but I haven't been able to reproduce it locally yet (still on it). It worked fine a few weeks ago when the last PR landed; might be a rustc regression.

jam1garner · 2020-04-13T16:17:25Z

So my motivation for this is, admittedly, somewhat ad-hoc in nature. After having a lot of issues with git (primarily upstream issues from cargo and libgit2), I've found builds to be easier for users to reproduce through XARGO_RUST_SRC, but given I:

Don't want to further complicate the build process
Don't want to be dependent on something like cargo-make
Would rather not use a introduce a makefile or shell script or anything along those lines

I can't really set the Rust source path. Having a more permanent per-project means of configuring this felt appropriate.

As far as why the [package] section:

Consistency with Cargo.toml
Indicates that it is a package-level configuration
Root-level string configurations in toml seem to not be idiomatic (at least from what I've seen from usage of tomls in the Rust community)
Xargo's toml doesn't seem to have any top-level configuration yet (unless I'm missing something, I didn't do the deepest dive on the code) and having top-level configurations being "loose" feels both subjectively wrong and a bit short-sighted, as if any other such configurations are added they'd probably want to be grouped in some manner

I'm open to any suggestions as to how to better integrate something that serves these needs while fitting the existing structure of the project, but if this style of change is just not a good fit for Xargo I certainly don't mind if I have to continuously maintain this soft fork for my users.

Regardless, thanks for taking time to consider my PR!

RalfJung · 2020-04-15T07:40:13Z

After having a lot of issues with git (primarily upstream issues from cargo and libgit2), I've found builds to be easier for users to reproduce through XARGO_RUST_SRC

I am very confused... you are working around cargo issues by using xargo, but would not otherwise need xargo? Do you have a reference for where these upstream issues you mentioned are tracked? Seems better to fix them than to accumulate more hacks elsewhere in the ecosystem. ;)

Also your example Xargo.toml does not look very portable; what kind of setup do you envision where the same XARGO_RUST_SRC will work on many different machines? That would be my other concern -- typically the exact location where the sources reside is different from platform to platform and even from machine to machine, so having it in a version-controlled file seems counterproductive.

Xargo's toml doesn't seem to have any top-level configuration yet

Yeah and that is exactly why I am wary of adding one, in this stage of xargo (maintenance-only, I am here mostly because I care about xargo working for the sake of miri.)

jam1garner · 2020-04-15T08:42:03Z

I am very confused... you are working around cargo issues by using xargo, but would not otherwise need xargo?

I am using xargo for providing an stdlib implementation for an otherwise unsupported platform. I was mentioning my motivation for this PR, not for using xargo in general.

Do you have a reference for where these upstream issues you mentioned are tracked?

Yes, not having shallow clones is actually the main issue that comes to mind -- rust-lang/cargo#1171

Seems better to fix them than to accumulate more hacks elsewhere in the ecosystem. ;)

Y'know what? Fair. I'm afraid I don't quite have the time or motivation to try and make such large contributions to libgit2 (and then cargo) as would be required to push this along while the issue is still relevant to me. (Which, to be clear, is on me_

Also your example Xargo.toml does not look very portable; what kind of setup do you envision where the same XARGO_RUST_SRC will work on many different machines? That would be my other concern -- typically the exact location where the sources reside is different from platform to platform and even from machine to machine, so having it in a version-controlled file seems counterproductive.

I mean yeah of course absolute paths aren't robust (admittedly the example I gave in the original comment was just the output of pwd that I had thrown into my toml for testing). What I actually use now is:

[package]
rust-src = "../rust-std-skyline-squashed/src"

The idea is that, for my use case, a user has a lot of projects linking against the same libstd and would like to ensure they are sharing the same copy (to save space and for control over updating). And, for other projects, including the rust source a submodule is reasonable.

RalfJung · 2020-04-15T08:56:18Z

I am using xargo for providing an stdlib implementation for an otherwise unsupported platform. I was mentioning my motivation for this PR, not for using xargo in general.

That is very useful information, thanks. Indeed it seems quite reasonable to not rely on xargo figuring out where the normal rust std source are as those are not very useful to you anyway. Suddenly it feels much less like a hack. ;)

Is rust-std-skyline-squashed in a git repo somewhere to look at, just to get a feel for it? You mention "including the rust source as a submodule"; I am not sure if you literally mean the full rustc repo with patches or something that just contains whatever libstd needs.

The idea is that, for my use case, a user has a lot of projects linking against the same libstd and would like to ensure they are sharing the same copy (to save space and for control over updating). And, for other projects, including the rust source a submodule is reasonable.

Okay, so (for the cases where you don't use submodules) you are expecting the user to perform some setup steps to get a matching folder layout. Makes sense.

Honestly the submodule case sounds even more convincing to me, that could then even be entirely self-contained: git clone, submodule setup, xargo build, done.

jam1garner · 2020-04-15T11:27:05Z

Is rust-std-skyline-squashed in a git repo somewhere to look at, just to get a feel for it?

Yep! you can find it here: https://github.com/jam1garner/rust-std-skyline-squashed, it's a Github Actions squashed repo created from here: https://github.com/jam1garner/rust-std-skyline. Honestly it's nothing special, just a fork + squash of the rust-lang/rust repo.

Okay, so (for the cases where you don't use submodules) you are expecting the user to perform some setup steps to get a matching folder layout.

Yep!

Honestly the submodule case sounds even more convincing to me, that could then even be entirely self-contained: git clone, submodule setup, xargo build, done.

I certainly agree. If I didn't expect people to have a g good number of projects at once depending on my stdlib, I would absolutely have gone that route.

RalfJung · 2020-04-15T11:29:11Z

Yep! you can find it here: https://github.com/jam1garner/rust-std-skyline-squashed, it's a Github Actions squashed repo created from here: https://github.com/jam1garner/rust-std-skyline. Honestly it's nothing special, just a fork + squash of the rust-lang/rust repo.

Interesting.
So, assuming the cargo issue with shallow clones wouldn't exist, what would you do instead? You said this motivates having control over the source location in Xargo.toml, but to me it seems like even if you could use https://github.com/jam1garner/rust-std-skyline, you would have all the same problems?

RalfJung · 2020-06-10T07:23:37Z

src/cargo.rs

@@ -46,7 +46,7 @@ impl Rustflags {
    pub fn for_xargo(&self, home: &Home) -> String {
        let mut flags = self.flags.clone();
        flags.push("--sysroot".to_owned());
-        flags.push(home.display().to_string());
+        flags.push(format!("\"{}\"", home.display()));


Isn't this RUSTFLAGS? The quotes won't work there. There currently just is no way to pass arguments with spaces via RUSTFLAGS. That is a cargo bug: rust-lang/cargo#6139

Oh I'm sorry I forgot I never closed this PR, I was trying to fix some random person's issue and must've pushed to master, my bad! Thank you for the additional info!

jam1garner · 2020-06-10T19:26:16Z

To clarify: I've since moved from one xargo wrapper (cargo-nro) to my own (cargo-skyline), which allows me to set XARGO_RUST_SRC every build, so my use case no longer needs this. Thank you a ton for taking a look and being open to discussing it!

RalfJung · 2020-06-11T07:34:19Z

Wow I had no idea there are xargo wrappers out there, let alone several of them... I hope they all have appropriate warnings that they rely on a tool that's basically unmaintained.^^

jam1garner added 5 commits April 12, 2020 13:51

Allow fallback on value package.rust-src if XARGO_RUST_SRC doesn't exist

cc6eee0

Fix failure to canonicalize path

29168ef

Attempt to fix windows paths

1ce938f

Print out path

f9b8e7f

Fix windows absolute path handling

b379147

RalfJung reviewed Apr 13, 2020

View reviewed changes

src/rustc.rs Outdated Show resolved Hide resolved

Switch Src::from into Src::new

1a26e56

jam1garner requested a review from RalfJung April 13, 2020 16:17

Try and fix bug with paths with spaces in them

663993e

RalfJung reviewed Jun 10, 2020

View reviewed changes

jam1garner closed this Jun 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow fallback on value package.rust-src if XARGO_RUST_SRC doesn't exist #288

Allow fallback on value package.rust-src if XARGO_RUST_SRC doesn't exist #288

jam1garner commented Apr 12, 2020

RalfJung commented Apr 13, 2020

RalfJung commented Apr 13, 2020

jam1garner commented Apr 13, 2020

RalfJung commented Apr 15, 2020

jam1garner commented Apr 15, 2020

RalfJung commented Apr 15, 2020

jam1garner commented Apr 15, 2020

RalfJung commented Apr 15, 2020

RalfJung Jun 10, 2020 •

edited

Loading

jam1garner Jun 10, 2020

jam1garner commented Jun 10, 2020

RalfJung commented Jun 11, 2020

Allow fallback on value package.rust-src if XARGO_RUST_SRC doesn't exist #288

Allow fallback on value package.rust-src if XARGO_RUST_SRC doesn't exist #288

Conversation

jam1garner commented Apr 12, 2020

RalfJung commented Apr 13, 2020

RalfJung commented Apr 13, 2020

jam1garner commented Apr 13, 2020

RalfJung commented Apr 15, 2020

jam1garner commented Apr 15, 2020

RalfJung commented Apr 15, 2020

jam1garner commented Apr 15, 2020

RalfJung commented Apr 15, 2020

RalfJung Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

jam1garner Jun 10, 2020

Choose a reason for hiding this comment

jam1garner commented Jun 10, 2020

RalfJung commented Jun 11, 2020

RalfJung Jun 10, 2020 •

edited

Loading