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

When a .cargo/config target-dir is set, xargo may fail #286

Closed
Firstyear opened this issue Apr 1, 2020 · 11 comments · Fixed by #310
Closed

When a .cargo/config target-dir is set, xargo may fail #286

Firstyear opened this issue Apr 1, 2020 · 11 comments · Fixed by #310

Comments

@Firstyear
Copy link

When a cargo target-dir is set, such as:

 > cat ~/.cargo/config
[build]
target-dir = "/Volumes/ramdisk/rs"

Xargo can fail, as the target directory is not where it expects, which can cause the cp_r utilities to fail unexpectedly. This was found in: rust-lang/miri#1285

@RalfJung
Copy link
Collaborator

RalfJung commented Apr 1, 2020

I wonder if this is a duplicate of #195.

Also, copying what I said on the Miri side:

I doubt an absolute target-dir can work for xargo, we kind of require a clean dir to know what it is that we have to copy (everything in the target dir) -- and because otherwise cargo thinks it can be smart and skip the build of stuff that is already built, which might be why we see no build for you.

Given that xargo anyway prepares a temporary directory for the sysroot build, I think what we should do is overwrite target-dir. Is that possible?

@Firstyear
Copy link
Author

You could put in a .cargo/config into every build you generate, because it uses the "most local" .cargo/config possible?

@RalfJung
Copy link
Collaborator

RalfJung commented Apr 1, 2020

According to https://doc.rust-lang.org/cargo/reference/config.html#environment-variables, env vars take precedence, so we could also try setting CARGO_TARGET_DIR.

@Firstyear
Copy link
Author

I guess part of the problem is, the user has requested you use target-dir for some reason - for me it's to reduce burn-out of my SSD due to huge and frequent builds taking write cycles so I build to a ram disk.

I think better, would be a way to make xargo work under the target dir that is set, rather than fighting to set it to something you want?

@Firstyear
Copy link
Author

Another thought I had this morning was that this would affect distributions who attempt to package or use CARGO_TARGET_DIR in some way for their build artifacts. So I think finding a way to respect the target-dir would be really good. Maybe if you need a guaranteed clean workspace, something like target-dir/xargo.HASH (you currently do TMPDIR/xargo.HASH I think ).

Would that be a reasonable suggestion? How would cargo clean affect or work with this?

@RalfJung
Copy link
Collaborator

RalfJung commented Apr 2, 2020

So I think finding a way to respect the target-dir would be really good.

I don't think it makes any sense. xargo builds in a temporary directory that it removes again when it is done. So how would there be any point in trying to follow local configuration about storing build artifacts elsewhere? It needs to know exactly where the build output is to process it further.

How would cargo clean affect or work with this?

xargo has its own workspace for building libstd. There is no interaction whatsoever with the workspace of your project. That's why I keep saying xargo should just overwrite the target dir.

@RalfJung
Copy link
Collaborator

RalfJung commented Apr 2, 2020

I guess part of the problem is, the user has requested you use target-dir for some reason - for me it's to reduce burn-out of my SSD due to huge and frequent builds taking write cycles so I build to a ram disk.

In that case, shouldn't you put your TEMPDIR also on the ramdisk? If you do, xargo will build on that ramdisk even when it overwrites target-dir.

@Firstyear
Copy link
Author

In that case, shouldn't you put your TEMPDIR also on the ramdisk? If you do, xargo will build on that ramdisk even when it overwrites target-dir.

No? My tempdir is not the same place as where I want my rust builds to go, they are seperate places.

I don't think it makes any sense. xargo builds in a temporary directory that it removes again when it is done. So how would there be any point in trying to follow local configuration about storing build artifacts elsewhere? It needs to know exactly where the build output is to process it further.

But the location where build artifacts go does matter, even if they are temporary. As mentioned, it could be in my case a ramdisk, for others a volume in a vm with different caching or performance needs, or a package distributor who in a chroot only has a readable build directory, or a different area with more capacity than my /home. Who knows? The issue here is xargo makes assumptions about my host system that are not always valid, when my host system says clearly "please build to this location".

A different way to ask this:

Is there a harm to xargo ta build to the target-dir, even if those artifacts are only temporary? Is there a real benefit to ignoring the target-dir parameter and using tmpdir?

@RalfJung
Copy link
Collaborator

RalfJung commented Apr 3, 2020

No? My tempdir is not the same place as where I want my rust builds to go, they are seperate places.

I would just assume that if you are worried about SSD writes, surely you'd also be worried about tmpdir causing such writes?

Is there a harm to xargo ta build to the target-dir, even if those artifacts are only temporary?

I don't know how to make xargo work with a generic target-dir. Things get even worse when we cannot rely on the target-dir being empty before xargo runs -- currently, we assume everything in that target dir to be created by the sysroot build, which is how xargo does not need any special magic knowledge about sysroots to copy the right things.

Is there a real benefit to ignoring the target-dir parameter and using tmpdir?

Yes -- it means that the current, rather simple implementation strategy of xargo continues to work.

So, I can fix this bug by making xargo overwrite target-dir. Or else we can leave it unfixed as I do not know another way to fix it (nor do I have huge amounts of time to do the research for a more clever solution).

Anyone else is of course welcome to come up with and implement a more clever solution if they want. :)

@Firstyear
Copy link
Author

No? My tempdir is not the same place as where I want my rust builds to go, they are seperate places.

I would just assume that if you are worried about SSD writes, surely you'd also be worried about tmpdir causing such writes?

Yes, but my tmpdir is generally used by my email, not rust builds that often have multi-hundred MB binaries being written out on a high frequency ... rust is really IO intensive for builds.

Is there a harm to xargo ta build to the target-dir, even if those artifacts are only temporary?

I don't know how to make xargo work with a generic target-dir. Things get even worse when we cannot rely on the target-dir being empty before xargo runs -- currently, we assume everything in that target dir to be created by the sysroot build, which is how xargo does not need any special magic knowledge about sysroots to copy the right things.

Okay I get that but why not use the target-dir and then put in a .xargo.hash ...

IE:

Current you do:
TMPDIR/xargo.hash/<content>
Why not:
cargo-target-dir/xargo.hash/<content>

IE read the current target dir from env or the .cargo/config, and then just set your target dir to "cargo_target_dir/xargo.hash". That has the same properties you want, you know it's your directory, and clean etc, and it has the properties I want of being in the "cargo dir". I think this would be a simple way to achieve both our goals.

Is there a real benefit to ignoring the target-dir parameter and using tmpdir?

Yes -- it means that the current, rather simple implementation strategy of xargo continues to work.

So, I can fix this bug by making xargo overwrite target-dir. Or else we can leave it unfixed as I do not know another way to fix it (nor do I have huge amounts of time to do the research for a more clever solution).

Anyone else is of course welcome to come up with and implement a more clever solution if they want. :)

Is the solution above simple enough? I think simple is better than clever here :)

But thanks for taking all the time to explain this to me, I appreciate it!

@RalfJung
Copy link
Collaborator

RalfJung commented Apr 6, 2020

Why not:
cargo-target-dir/xargo.hash/

I don't know if tmpdir, the crate we use for getting a temporary directory, supports that. It is really convenient to use, but not meant to cover weird special cases like this one. ;)

Also keep in mind that "normally", cargo-target-dir is relative (at least, the documentation explicitly says "relative to the workspace directory"). So we'd need to add some extra code to treat relative and absolute target-dirs separately. And then there is the problem that I don't know of a supported way in cargo to ask for the target-dir that is going to be used (there can be overrides on the project and user level and in the environment and whatnot).

It's certainly possible, but it's a lot of complexity. I don't agree with your assessment that this is "simple".

bors bot added a commit that referenced this issue Jan 15, 2021
310: Fix copy failure by overriding CARGO_TARGET_DIR r=RalfJung a=oxalica

Fixes #286 

According to @RalfJung (#286 (comment)). It's better to use default `target`. So we just need to explicit override the target directory to fix the copy failure.

> xargo has its own workspace for building libstd. There is no interaction whatsoever with the workspace of your project. That's why I keep saying xargo should just overwrite the target dir.

Co-authored-by: oxalica <oxalicc@pm.me>
@bors bors bot closed this as completed in 23c6fd2 Jan 16, 2021
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 a pull request may close this issue.

2 participants