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.

Sign up for GitHub

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

Jump to bottom

Rename --out-dir to --artifact-dir #13809

Merged
merged 2 commits into from
Jun 7, 2024
Merged

Rename --out-dir to --artifact-dir #13809

merged 2 commits into from
Jun 7, 2024

Conversation

valadaptive
Copy link
Contributor

@valadaptive valadaptive commented Apr 26, 2024 •
edited by epage

Progress towards unblocking #6790. Renames the experimental --out-dir argument to --artifact-dir, both to reflect that it's where the final build artifacts will be copied to, and to avoid confusion with the OUT_DIR environment variable which serves an entirely different purpose.

For transition purposes, --out-dir argument and out-dir config key will still work with a deprecation message encouraging the use of the new arg and config key.

Rationale

A lot of people seem to be confused by the naming of the --out-dir argument, and are misled into thinking it serves the same purpose as the OUT_DIR environment variable:

However, it doesn't seem that OUT_DIR environment variable is set to the value of --out-dir when build.rs is executed.

I understand that the worry is that there could be confusion between --out-dir for cargo and the environment variable OUT_DIR for build.rs, but doesn't it mean exactly the same in both cases?

--out-dir: Things will be built into $PWD/target as normal, but copies some of the artifacts into the directory specified by out-dir (not a profile specific subdirectory). Unstable flag, added in March 2018. cargo build --out-dir #5203 Ability to specify output artifact name #4875. Mimicks the behavior of OUT_DIR.

I recently had a couple of people express an interest in --out-dir being stabilized and from my initial digging it seems like what they may actually want is to switch to OUT_DIR, which is already stable.

@rustbot
Copy link
Collaborator

rustbot commented Apr 26, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-documenting-cargo-itself Area: Cargo's documentation A-layout Area: target output directory layout, naming, and organization A-semver Area: semver specifications, version matching, etc. Command-build Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2024
@valadaptive valadaptive mentioned this pull request Apr 26, 2024
Open
9 tasks
@workingjubilee
Copy link

Grim news. Perhaps both flags must be briefly supported to deal with the bootstrap problem.

@valadaptive valadaptive force-pushed the artifact-dir branch 3 times, most recently from 4149c00 to 6737e08 Compare May 4, 2024 02:37
@valadaptive
Copy link
Contributor Author

Never mind; it seems I was a bit zealous in replacing --out-dir and accidentally search-and-replaced it in some rustc invocations. Might still need to do some fixups but we're looking a lot better now.

@rustbot rustbot added the A-configuration Area: cargo config files and env vars label May 4, 2024
@valadaptive valadaptive force-pushed the artifact-dir branch 5 times, most recently from 549b724 to 14624e8 Compare May 4, 2024 04:44
@valadaptive valadaptive marked this pull request as ready for review May 4, 2024 06:18
@valadaptive
Copy link
Contributor Author

This should be ready for review now. Not sure if this needs to go through any sort of design process since it's renaming a nightly-only API. There does seem to be some consensus in #6790 that --artifact-dir is a better name.

@valadaptive valadaptive changed the title WIP: rename out-dir to artifact-dir Rename --out-dir to --artifact-dir May 4, 2024
@valadaptive
Copy link
Contributor Author

@epage Just wanted to make sure this hasn't fallen off your radar--it's ready for review now.

@epage
Copy link
Contributor

epage commented May 21, 2024

I have about a month backlog of github notifications that I still need to respond to.

@epage
Copy link
Contributor

epage commented Jun 6, 2024

Could you update the commits for how you'd want them presented for review and merging?

@valadaptive
Rename out-dir to artifact-dir
4b309bc 
Per discussion in rust-lang#6790. The
--out-dir CLI option and out-dir config option are often confused with
the OUT_DIR environment variable, when the two serve very different
purposes (the former tells Cargo where to copy build artifacts to,
whereas the OUT_DIR environment variable is set *by* Cargo to tell
build scripts where to place their generated intermediate artifacts).
Renaming the option to something less confusing is a prerequisite to
stabilizing it.
@valadaptive
Add tests for out-dir option deprecation
0aac303 
Ensure that the old options still work and provide the proper
deprecation warning.
@valadaptive
Copy link
Contributor Author

I've updated the flag-checking code and squashed most of the commits.

Should the deprecation messages provide some sort of warning that the old options will eventually be removed, or are they fine currently?

@valadaptive valadaptive requested a review from epage June 7, 2024 07:35
epage
epage reviewed Jun 7, 2024
View reviewed changes
tests/testsuite/artifact_dir.rs
@@ -304,6 +304,55 @@ For more information, try '--help'.
.run();
}

#[cargo_test]
fn deprecated_out_dir() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not important enough for this change but in the future, I generally recommend adding the tests as the first commit (with them passing). Then as you change behavior, that is reflected in how the tests are changed to ensure they still pass. This makes it easier for reviewers to see how behavior changed, we can then show those diffs in posts to the community for them to understand changes, and it helps "test the test" (I've made changes where I thought I was testing the right condition and wasn't).

@epage
Copy link
Contributor

epage commented Jun 7, 2024

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 7, 2024

📌 Commit 0aac303 has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 7, 2024
@bors
Copy link
Collaborator

bors commented Jun 7, 2024

⌛ Testing commit 0aac303 with merge 4189f50...

@bors
Copy link
Collaborator

bors commented Jun 7, 2024

☀️ Test successful - checks-actions
Approved by: epage
Pushing 4189f50 to master...

@bors bors merged commit 4189f50 into rust-lang:master Jun 7, 2024
21 checks passed
@weihanglo weihanglo mentioned this pull request Jun 8, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 8, 2024
@bors
Auto merge of rust-lang#126143 - weihanglo:update-cargo, r=weihanglo
16e8803 
Update cargo

8 commits in 34a6a87d8a2330d8c9d578f927489689328a652d..b1feb75d062444e2cee8b3d2aaa95309d65e9ccd
2024-06-04 15:31:01 +0000 to 2024-06-07 20:16:17 +0000
- Keep lints updated (rust-lang/cargo#14030)
- test(lints): Ensure unused optional dep fires for shadowed dep (rust-lang/cargo#14028)
- Add `cargo update --breaking` (rust-lang/cargo#13979)
- Add tooling to document lints (rust-lang/cargo#14025)
- Rename --out-dir to --artifact-dir (rust-lang/cargo#13809)
- fix(lints): Add unknown_lints to lints list (rust-lang/cargo#14024)
- docs(contrib): Suggest atomic commits with separate test commits (rust-lang/cargo#14014)
- test(semver): track the behavior of `--precise <prerelease>` (rust-lang/cargo#14013)

r? ghost
@rustbot rustbot added this to the 1.81.0 milestone Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@epage epage

Labels
A-build-execution Area: anything dealing with executing the compiler A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation A-layout Area: target output directory layout, naming, and organization A-semver Area: semver specifications, version matching, etc. Command-build Command-fix S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@valadaptive @rustbot @workingjubilee @epage @bors