Throw error if CARGO_TARGET_DIR is an empty string

[Andy-Python-Programmer]

This pull request makes the target dir to be target/ if CARGO_TARGET_DIR is `` with spaces trimmed and not delete the current project.

Fixes: #8866

[rust-highfive]

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.

[alexcrichton]

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]

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:

[alexcrichton]

Thanks! Can you add a test for this as well?

Also, is there any reason that we shouldn't handle the other cases set to empty strings as well?

[jyn514]

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]

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}/ /

[alexcrichton]

@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]

=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.

[Andy-Python-Programmer]

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]

@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]

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]

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]

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]

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]

Ah yes that makes sense. I will just do that 🙃

[Andy-Python-Programmer]

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]

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

[joshtriplett]

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]

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]

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]

All good.

[Andy-Python-Programmer]

Now:

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]

@bors: r+

Thanks!

[bors]

📌 Commit b5b2e48 has been approved by alexcrichton

[bors]

⌛ Testing commit b5b2e48 with merge 439a2016b7306bfe948a788f89607e8cceb7876a...

[bors]

💔 Test failed - checks-actions

[alexcrichton]

@bors: retry

[bors]

⌛ Testing commit b5b2e48 with merge f3e63d6...

[bors]

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