Out of tree #109
Closed
Out of tree #109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We are going to want to do something more sophisticated than hardcode "Cargo.toml" and use of current_dir(). So: * builder_interface, metadata_deps: Replace hardcoded "Cargo.toml" with manifest_path. * metadata, builder_interface: Replace open-coded current_dir().unwrap() with manifest_dir. * metadata: Add call to .manifest_path(manifest_path). Currently manifest_path and manifest_dir are just "Cargo.toml" and current_dir().unwrap(), so there is no functional change. Splitting this out as a separate commit will make the actual functional change less full of noise.
These tests are invoked in the source directory. In an out of tree build, that may be totally different to the invocation directory. We really want to chdir back to the cargo invocation directory. Otherwise we risk (for example) picking up inappropriate cargo configuration somewhere in the parents of the source directory. This is not possible because cargo doesn't tell us what it is. For now, the best guess is to change to CARGO_HOME. That means at least that we will pick up the right cargo configuration. A consequence is that we must no longer use current_dir(), but rather the directory containing the manifest, which cargo supplies in CARGO_MANIFEST_DIR.
|
So this doesn't require any actual changes, you just want to make sure we don't regress this by adding a test? |
|
Oliver Scherer writes ("Re: [oli-obk/cargo_metadata] Out of tree (#109)"):
So this doesn't require any actual changes, you just want to make
sure we don't regress this by adding a test?
Sorry, perhaps I was unclear.
The existing tests fail for me in my usual build environment because
the nested copy of cargo picks up an inappropriate config, by hunting
up from the source directory. With my changes they pass. People
doing in-tree builds should find they still pass - indeed, I tested
that and also Travis agrees :-).
Let me make it a bit more concrete. If user alice does this:
cd ~/builds
mkdir cargo_metadata
cd cargo_metadata
cargo check --manifest-path=/home/bob/sources/cargo_metadata/Cargo.toml --target-dir=target
then alice's actual cargo invocation finds ~alice/.cargo/ and so uses
alice's cargo configuration. But the tests inside selftest.rs
themselves run cargo again. Effectively what happens is this:
# cargo test does this
[ run some builds ]
cd ~bob/sources/cargo_metadata
~alice/builds/cargo_metadata/.../selftest
# inside selftest.rs
cargo metadata
That copy of cargo looks in its cwd and parent directories - ie it
looks in the following places for .cargo and its configuration:
~bob/sources/cargo_metadata
~bob/sources/
~bob/
It then finds ~bob/.cargo and uses bob's configuration. This is
clearly wrong. In my case ~bob/.cargo has a configuration which
doesn't work for users other than bob. alice's cargo metadata
command bombs out and the test fails.
This MR fixes that by running the nested copy of cargo in $CARGO_HOME
rather than in the source directory. So instead, we have this:
# cargo test does this
[ run some builds ]
cd ~bob/sources/cargo_metadata
~alice/builds/cargo_metadata/.../selftest
# inside selftest.rs
cd ~alice/.cargo
cargo metadata --manifest-path=/home/bob/sources/cargo_metadata/Cargo.toml
And then it works. The nested cargo sees the config from
~alice/.cargo, which is correct, and uses the manifest file from
~bob/sources, which is also correct. It doesn't write anything to the
cwd so this is all file.
It would be better for the cd to be back to
~alice/builds/cargo_metadata/ but we don't know that directory.
But ~alice/.cargo (ie $CARGO_HOME) at least makes the nested cargo
pick up alice's cargo config rather than bob's.
Sorry to go on at such length. I hope this explains things...
Regards,
Ian.
…--
Ian Jackson <[email protected]> These opinions are my own.
If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
|
|
Ah wonderful. Can you add something to CI that will fail if we ever brick this? I feel like it's easy to break testing again for you in the future. |
|
Oliver Scherer writes ("Re: [oli-obk/cargo_metadata] Out of tree (#109)"):
Ah wonderful. Can you add something to CI that will fail if we ever brick this?
I feel like it's easy to break testing again for you in the future.
Tricky. I'm not very familiar with Travis. But I can take a look.
Thanks for the encouragement, anyway!
Regards,
Ian.
…--
Ian Jackson <[email protected]> These opinions are my own.
If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
|
|
Ian Jackson writes ("Re: [oli-obk/cargo_metadata] Out of tree (#109)"):
Tricky. I'm not very familiar with Travis. But I can take a look.
Thanks for the encouragement, anyway!
I have been playing with this and the problems seem deeper than I
thought. In particular, I find that when I wrote a more thorough test
for this situation, some of the doctests fail.
The reason for this is that the doctests suffer from the same issue:
the examples show .manifest_path("./Cargo.toml"). But if you actually
write code that does that it won't work right in out-of-tree builds.
I am tempted to suggest that the right fix is for *MetadataCommand*
to chdir to CARGO_HOME. By default, if no other .current_dir() is
supplied. And presumably a note in the documentation.
The result would be that the existing doctests (and, presumably,
everyone's code) would be made to work correctly. Ie, it seems to me
that we ought to provide an interface that does the right thing by
default.
What do you think ?
In the meantime I have closed the MR because my more thorough tests
seem to have revealed that my original solution was incomplete.
Regards,
Ian.
…--
Ian Jackson <[email protected]> These opinions are my own.
If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
|
|
phew... I'm not sure, and don't have the time to really dig into this. Do you think it will become better if your cargo issue is resolved? |
|
Oliver Scherer writes ("Re: [oli-obk/cargo_metadata] Out of tree (#109)"):
phew... I'm not sure, and don't have the time to really dig into this. Do you
think it will become better if your cargo issue is resolved?
It seems to me that changing back to the cargo original invocation
directory, if cargo supplied that in an env var, is something that
would definitely be sensible to do in MetadataCommand by default.
It would only happen if the env var was set - so, only when
cargo_metadata was being used from something run from within cargo.
In that case running the nested cargo from the same directory as the
original cargo invocation is clearly more correct and more likely to
produce correct results.
I *think* this is true of changing to CARGO_HOME, too. But I am less
sure of the implications of that.
Ian.
…--
Ian Jackson <[email protected]> These opinions are my own.
If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi. I like to do builds out of tree, with the actual source code not writeable by the build process. This is achieved with cargo --manifest-path=..../Cargo.toml --target-dir=target. This does not work right now if one of the parents of the source code directory has its own cargo config: the cargo run by the cargo_metadata tests picks up the wrong cargo config.
In this MR I propose to fix this by changing directory to $CARGO_HOME. This is not ideal: it would be better to change back to the invocation directory, but the invocation directory is not currently available (rust-lang/cargo#8148)
I have checked that "cargo test" still works in both in-tree and out-of-tree builds.