Github Throw error if CARGO_TARGET_DIR is an empty string by Andy-Python-Program...

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Thanks for the PR! I'm a bit uncomfortable reinterpreting a request for CARGO_TARGET_DIR, however. It seems like the bug would rather be whomever set this env var because they probably didn't mean for an empty string.

Could this perhaps instead return an error?

Andy-Python-Programmer commented on Dec 5, 2020 •

Yeup @alexcrichton. It makes sense that if the env variable is empty is probably the users fault. So returning an error makes more sense!

I have made the changes. Now it returns a error:

src/cargo/util/config/mod.rs

Outdated

Show resolved

jyn514 commented on Dec 13, 2020 •

Thanks for the PR! I'm a bit uncomfortable reinterpreting a request for CARGO_TARGET_DIR, however. It seems like the bug would rather be whomever set this env var because they probably didn't mean for an empty string.

Could this perhaps instead return an error?

I would prefer not to do this. The Unix convention is for empty variables to act as if they're unset - if you look at how PAGER and GIT_PAGER behave, it means you can use PAGER= git status as a shorthand for OLD_PAGER=$PAGER; unset PAGER; git status; PAGER=$OLD_PAGER; unset OLD_PAGER;. That second command is awfully long to run.

See also dandavison/delta#386.

Andy-Python-Programmer commented on Dec 14, 2020 •

I guess then

if dir.to_str()? == "" {
    Ok(Some(Filesystem::new(self.cwd.join("target"))))
}

is the right thing to do. As if we trim it then its actually users mistake and we do not need to check that as when we exe a command like cargo clean then it should return that no dir named {workSpace}/ /

@jyn514 can you explain when you think the build directory may wish to be configured in a one-off context like that? And why =target would not suffice?

jyn514 commented on Dec 14, 2020

=target would work, I'd be ok with the error if it suggests doing that instead.

I use this for testing cargo plugins. I have a global CARGO_TARGET_DIR set to an absolute path, which breaks lots and lots of tools that assume target is always the output directory. Either to workaround that breakage, or to test fixes to the command around CARGO_TARGET_DIR, I set the variable one-off a fair bit to make sure this one command has the correct variable without having to rebuild all my other projects now that the target/ directory isn't cached. You can see an example of that here: deadlinks/cargo-deadlinks#66.

So should I make it return an error, make it behave as if the variable is unset or unset the variable and run the program as normal. @alexcrichton

alexcrichton commented on Jan 4

@Andy-Python-Programmer the case just below the one you modified, accessing self.build_config()?.target_dir I think should probably also handle the empty-string logic

Andy-Python-Programmer commented on Jan 11 •

Andy-Python-Programmer the case just below the one you modified, accessing self.build_config()?.target_dir I think should probably also handle the empty-string logic

@alexcrichton I am not sure as we should only handle the case when the user has set the env var. Cause target will be "" only when the user sets it to be "" right?

alexcrichton commented on Jan 14

I'm personally not sure why we would only handle the case of the env var. Why would the env var require special handling but not other methods of configuration?

Andy-Python-Programmer commented on Jan 14

Why would the env var require special handling but not other methods of configuration?

Because we cannot set the config variable in the other methods of config to `` I guess.

alexcrichton commented on Jan 19

Sorry I'm not really understanding your responses. Let me try to be more clear.

If you set export CARGO_TARGET_DIR=, then this PR will generate an error next time you build.

My point is that if you set:

[build]
target-dir = '' 

or export CARGO_BUILD_TARGET_DIR=, thten this PR should also generate an error. Currently it does not.

Andy-Python-Programmer commented on Jan 19

Ah yes that makes sense. I will just do that

Andy-Python-Programmer commented on Jan 23 •

Now if you set

[build]
target-dir = '' 

or CARGO_TARGET_DIR to "", you will get an error error: the target directory is set to an empty string. Is that more like it? I also added tests.

alexcrichton commented on Jan 25

Could you be sure to add a test for the config case and a test for the env case as well?

joshtriplett commented 22 days ago

Following up on this: I think this is a good idea, and I think we should merge this as soon as we have tests for the other cases.

Andy-Python-Programmer commented 21 days ago

Just a question =>

#[cargo_test]
fn cargo_target_empty_env() {
    let config = ConfigBuilder::new().env("CARGO_TARGT_DIR", "").build();

for the env test I think this is the right way to set the env in the config. So will I need to add a case to also check self.env?

alexcrichton commented 20 days ago

It may make more sense to have more of an "integration test" for this where you build a whole project and execute cargo build. That way you can simulate the env var with .env(...) when building the subprocess and the config version can be emulated by creating a file.

Andy-Python-Programmer commented 18 days ago

All good.

Andy-Python-Programmer commented 18 days ago •

Case 1

If you set the environment variable to "". It will error out saying:
error: the target directory is set to an empty string in the CARGO_TARGET_DIR environment variable.

Case 2

[build]
target-dir = '' 

If you set the target diectory to "" in the config file it will error out saying:
error: the target directory is set to an empty string in the config.toml file.

I hope I did this right this time :)

alexcrichton commented 17 days ago

@bors: r+

Thanks!

bors commented 17 days ago

Commit b5b2e48 has been approved by alexcrichton

bors commented 17 days ago

Testing commit b5b2e48 with merge 439a201...

bors commented 17 days ago

Test failed - checks-actions

alexcrichton commented 17 days ago

@bors: retry

bors commented 17 days ago

Testing commit b5b2e48 with merge f3e63d6...

bors commented 17 days ago

Test successful - checks-actions
Approved by: alexcrichton
Pushing f3e63d6 to master...