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

In addition to CARGO_TARGET_DIR env var, support --target-dir command line option #5308

Closed
matklad opened this issue Apr 6, 2018 · 10 comments
Labels
E-easy Experience: Easy

Comments

@matklad
Copy link
Member

matklad commented Apr 6, 2018

@nirbheek you wanted this for messon integration. Could you elaborate on why do you think it is better than an environmet variable?

@rust-lang/cargo it seems that such addition would be E-easy and straightforward? The only complication I can think of are build scripts which invoke cargo recursively for whatever reason, but we can just set the env car for them manually. Should we implement this? If yes, I’ll write some mentoring instructions :)

@alexcrichton
Copy link
Member

Seems reasonable to me to implement!

@nirbheek
Copy link

nirbheek commented Apr 6, 2018

Could you elaborate on why do you think it is better than an environment variable?

Speaking generally, in my opinion command-line arguments are more explicit and user-friendly than environment variables. They are easier to change or discover, and are less likely to have unintended side-effects.

@matklad matklad added E-easy Experience: Easy E-help-wanted labels Apr 6, 2018
@matklad
Copy link
Member Author

matklad commented Apr 6, 2018

So, if anyone wants to implement this, here are some pointers to code:

  1. To specify command line argment itself, you should define a helper function over here and than call it for all relevant commands. I think that all commands that call arg_target_triple or arg_jobs would probably want to call arg_target_dir as well

  2. The code to calculate target_dir is here

  3. I think that probably we can thread value of command line argument to config via this call?

  4. The test for target dir is here

@smithsps
Copy link
Contributor

smithsps commented Apr 9, 2018

@matklad I would be interested in working on this. What would be the best way to direct questions?

@matklad
Copy link
Member Author

matklad commented Apr 9, 2018

@smithsps awesome!

You can ask the questions right here, and we have an IRC channel as well!

@smithsps
Copy link
Contributor

Do we only need to create a single test in build that accompanies the existing one? Or do we need one with every command we include arg_target_dir?

@matklad
Copy link
Member Author

matklad commented Apr 10, 2018

A single dest would be enogh!

@smithsps
Copy link
Contributor

To check in, I've got a working solution.
But, I'm not sure its a complete architectural fit. Could you check if my approach is correct?

I'll probably pull request shortly, but I still need to double check the correct subcommands and their various description.

bors added a commit that referenced this issue Apr 24, 2018
Add target directory parameter --target-dir

Implements: #5308

Adds a target directory parameter, that acts in the same manner as the environment variable `CARGO_TARGET_DIR`, to the following subcommands:
- `bench`
- `build`
- `check`
- `clean`
- `doc`
- `package`
- `publish`
- `run`
- `rustc`
- `rustdoc`
- `test`
@dwijnand
Copy link
Member

Fixed by #5393

@alexcrichton
Copy link
Member

Indeed, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Experience: Easy
Projects
None yet
Development

No branches or pull requests

5 participants