Skip to content

Change config paths to only check CARGO_HOME for cargo-script#14749

Merged
bors merged 1 commit intorust-lang:masterfrom
0xPoe:rustin-patch-cargo-script
Nov 4, 2024
Merged

Change config paths to only check CARGO_HOME for cargo-script#14749
bors merged 1 commit intorust-lang:masterfrom
0xPoe:rustin-patch-cargo-script

Conversation

@0xPoe
Copy link
Member

@0xPoe 0xPoe commented Oct 30, 2024 •
edited
Loading

What does this PR try to resolve?

ref #12207 (comment)
After this change, we will only check the configs from the CARGO_HOME.

How should we test and review this PR?

The test has been updated.

Additional information

r? @epage

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-run S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2024
@0xPoe
Copy link
Member Author

0xPoe commented Oct 30, 2024

@epage Even I found the code and changed it. But I am a little confused about your comments.

When we talked about this as a Cargo team, we decided this need to reload to cargo home, like cargo install does, to avoid downloading a script and being subject to spurious files in your ~/Downloads, at the cost of losing the config from your repo.

What do you mean by downloading a script? And what kind of spurious files might end up being created? I thought it only read the config files.

@epage
Copy link
Contributor

epage commented Oct 30, 2024

What do you mean by downloading a script? And what kind of spurious files might end up being created? I thought it only read the config files.

If I download a script off the internet, my browser will put it in ~/Downloads. That is a grab bag directory with who knows what in it. A user might have extracted a project into that directory to look at that could have dropped a .cargo/config.toml into it. The concern is with that being used when it wasn't intended. That unfortunately means we'll not load any config for cargo scripts.

@0xPoe 0xPoe force-pushed the rustin-patch-cargo-script branch from c00c27a to cbb9dae Compare October 31, 2024 14:29
@0xPoe 0xPoe marked this pull request as ready for review October 31, 2024 14:32
0xPoe
0xPoe commented Oct 31, 2024
View reviewed changes
Copy link
Member Author

@0xPoe 0xPoe left a comment

Choose a reason for hiding this comment

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

🔢 Self-check (PR reviewed by myself and ready for feedback.)

epage
epage reviewed Oct 31, 2024
View reviewed changes
@epage epage mentioned this pull request Oct 31, 2024
Open
39 tasks
@0xPoe 0xPoe force-pushed the rustin-patch-cargo-script branch from cbb9dae to a61d845 Compare November 3, 2024 10:04
0xPoe

This comment was marked as outdated.

Sign in to view

@0xPoe 0xPoe force-pushed the rustin-patch-cargo-script branch 2 times, most recently from a2d6143 to 09faa3e Compare November 3, 2024 10:09
@0xPoe
Change config paths to only check CARGO_HOME for cargo-script
bd47da1 
Signed-off-by: Rustin170506 <29879298+Rustin170506@users.noreply.github.com>
@0xPoe 0xPoe force-pushed the rustin-patch-cargo-script branch from 09faa3e to bd47da1 Compare November 3, 2024 10:12
0xPoe
0xPoe commented Nov 3, 2024
View reviewed changes
Copy link
Member Author

@0xPoe 0xPoe left a comment

Choose a reason for hiding this comment

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

🔢 Self-check (PR reviewed by myself and ready for feedback.)

@0xPoe 0xPoe requested a review from epage November 3, 2024 10:14
@epage
Copy link
Contributor

epage commented Nov 4, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2024

📌 Commit bd47da1 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 Nov 4, 2024
@bors
Copy link
Contributor

bors commented Nov 4, 2024

⌛ Testing commit bd47da1 with merge ad3cdb4...

@bors
Copy link
Contributor

bors commented Nov 4, 2024

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

@bors bors merged commit ad3cdb4 into rust-lang:master Nov 4, 2024
@0xPoe
Copy link
Member Author

0xPoe commented Nov 5, 2024

Thanks for your review! 💚 💙 💜 💛 ❤️

@weihanglo weihanglo mentioned this pull request Nov 9, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2024
@bors
Auto merge of rust-lang#132829 - weihanglo:update-cargo, r=weihanglo
303fc05 
Update cargo

16 commits in 0310497822a7a673a330a5dd068b7aaa579a265e..4a2d8dc636445b276288543882e076f254b3ae95
2024-11-01 19:27:56 +0000 to 2024-11-09 19:10:33 +0000
- test: adjust `cargo_test_env` to unblock rust submodule update (rust-lang/cargo#14803)
- feat(warnings): add build.warnings option (rust-lang/cargo#14388)
- Revert "feat: Add `CARGO_RUSTC_CURRENT_DIR`" (rust-lang/cargo#14799)
- CI: make the `lint-docs` job required (rust-lang/cargo#14797)
- Switch CI from bors to merge queue (rust-lang/cargo#14718)
- docs(test):  Document Execs assertions based on port effort (rust-lang/cargo#14793)
- fix(test): Make redactions consistent with snapbox (rust-lang/cargo#14790)
- test(gc): Update remaining unordered tests to snapbox (rust-lang/cargo#14785)
- Normalize the `target` paths (rust-lang/cargo#14497)
- rustfix: replace special-case duplicate handling with error (rust-lang/cargo#14782)
- test: Update some emaining unordered tests to snapbox (rust-lang/cargo#14781)
- Change config paths to only check CARGO_HOME for cargo-script (rust-lang/cargo#14749)
- Enable transfer feature in triagebot (rust-lang/cargo#14777)
- Add transactional semantics to `rustfix` (rust-lang/cargo#14747)
- doc: fix `GlobalContext` reference (rust-lang/cargo#14773)
- chore: update handlebars to v6, fix build error (rust-lang/cargo#14772)
@rustbot rustbot added this to the 1.84.0 milestone Nov 10, 2024
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
@bors @mati865
Auto merge of rust-lang#132829 - weihanglo:update-cargo, r=weihanglo
b5e0764 
Update cargo

16 commits in 0310497822a7a673a330a5dd068b7aaa579a265e..4a2d8dc636445b276288543882e076f254b3ae95
2024-11-01 19:27:56 +0000 to 2024-11-09 19:10:33 +0000
- test: adjust `cargo_test_env` to unblock rust submodule update (rust-lang/cargo#14803)
- feat(warnings): add build.warnings option (rust-lang/cargo#14388)
- Revert "feat: Add `CARGO_RUSTC_CURRENT_DIR`" (rust-lang/cargo#14799)
- CI: make the `lint-docs` job required (rust-lang/cargo#14797)
- Switch CI from bors to merge queue (rust-lang/cargo#14718)
- docs(test):  Document Execs assertions based on port effort (rust-lang/cargo#14793)
- fix(test): Make redactions consistent with snapbox (rust-lang/cargo#14790)
- test(gc): Update remaining unordered tests to snapbox (rust-lang/cargo#14785)
- Normalize the `target` paths (rust-lang/cargo#14497)
- rustfix: replace special-case duplicate handling with error (rust-lang/cargo#14782)
- test: Update some emaining unordered tests to snapbox (rust-lang/cargo#14781)
- Change config paths to only check CARGO_HOME for cargo-script (rust-lang/cargo#14749)
- Enable transfer feature in triagebot (rust-lang/cargo#14777)
- Add transactional semantics to `rustfix` (rust-lang/cargo#14747)
- doc: fix `GlobalContext` reference (rust-lang/cargo#14773)
- chore: update handlebars to v6, fix build error (rust-lang/cargo#14772)
@epage epage mentioned this pull request Nov 15, 2024
Open
11 tasks
epage added a commit to epage/cargo that referenced this pull request Feb 10, 2026
@epage
fix(script): Load config relative to the script
4d0cd89 
This was the original behavior.
There was some concern over this previously and we switched to only
loading config from CARGO_HOME in rust-lang#14749.
After discussing this in today's cargo team meeting, we decided to
switch it back.

A concern brought up previously was if you previously downloaded a
config and now download and run a script, you could get surprising
behavior, maybe even dangerous.
This does require some extra hoops because you don't have a
`.cargo.toml` but a `.cargo/config.toml`.
You can't directly download that but must first download a zip file and
then decompress it without it having a parent directory.

Contrast that with users who have a script in their repo and config that
should apply to it.
This is an important use case but one we will get less feedback on
during calls for testing.

In discussing this, we felt there are different use cases:
- The repo with a config file
- People surprised at how config loading works

We settled on this change because it is consistent with the current
behavior, even if it can be confusing and undesirable.
We can then work to improve the overall config search path experience
and both regular cargo commands and running of cargo scripts would
benefit.

This reverts commit bd47da1.
@epage epage mentioned this pull request Feb 10, 2026
Merged
epage added a commit to epage/cargo that referenced this pull request Feb 10, 2026
@epage
fix(script): Load config relative to the script
f98b908 
This was the original behavior.
There was some concern over this previously and we switched to only
loading config from CARGO_HOME in rust-lang#14749.
After discussing this in today's cargo team meeting, we decided to
switch it back.

A concern brought up previously was if you previously downloaded a
config and now download and run a script, you could get surprising
behavior, maybe even dangerous.
This does require some extra hoops because you don't have a
`.cargo.toml` but a `.cargo/config.toml`.
You can't directly download that but must first download a zip file and
then decompress it without it having a parent directory.

Contrast that with users who have a script in their repo and config that
should apply to it.
This is an important use case but one we will get less feedback on
during calls for testing.

In discussing this, we felt there are different use cases:
- The repo with a config file
- People surprised at how config loading works

We settled on this change because it is consistent with the current
behavior, even if it can be confusing and undesirable.
We can then work to improve the overall config search path experience
and both regular cargo commands and running of cargo scripts would
benefit.

This reverts commit bd47da1.
epage added a commit to epage/cargo that referenced this pull request Feb 10, 2026
@epage
fix(script): Load config relative to the script
946583f 
This was the original behavior.
There was some concern over this previously and we switched to only
loading config from CARGO_HOME in rust-lang#14749.
After discussing this in today's cargo team meeting, we decided to
switch it back.

A concern brought up previously was if you previously downloaded a
config and now download and run a script, you could get surprising
behavior, maybe even dangerous.
This does require some extra hoops because you don't have a
`.cargo.toml` but a `.cargo/config.toml`.
You can't directly download that but must first download a zip file and
then decompress it without it having a parent directory.

Contrast that with users who have a script in their repo and config that
should apply to it.
This is an important use case but one we will get less feedback on
during calls for testing.

In discussing this, we felt there are different use cases:
- The repo with a config file
- People surprised at how config loading works

We settled on this change because it is consistent with the current
behavior, even if it can be confusing and undesirable.
We can then work to improve the overall config search path experience
and both regular cargo commands and running of cargo scripts would
benefit.

This reverts commit bd47da1.
epage added a commit to epage/cargo that referenced this pull request Feb 10, 2026
@epage
fix(script): Load config relative to the script
8207016 
This was the original behavior.
There was some concern over this previously and we switched to only
loading config from CARGO_HOME in rust-lang#14749.
After discussing this in today's cargo team meeting, we decided to
switch it back.

A concern brought up previously was if you previously downloaded a
config and now download and run a script, you could get surprising
behavior, maybe even dangerous.
This does require some extra hoops because you don't have a
`.cargo.toml` but a `.cargo/config.toml`.
You can't directly download that but must first download a zip file and
then decompress it without it having a parent directory.

Contrast that with users who have a script in their repo and config that
should apply to it.
This is an important use case but one we will get less feedback on
during calls for testing.

In discussing this, we felt there are different use cases:
- The repo with a config file
- People surprised at how config loading works

We settled on this change because it is consistent with the current
behavior, even if it can be confusing and undesirable.
We can then work to improve the overall config search path experience
and both regular cargo commands and running of cargo scripts would
benefit.

This reverts commit bd47da1.
github-merge-queue bot pushed a commit that referenced this pull request Feb 11, 2026
@weihanglo
fix(script): Load config relative to the script (#16620)
7de468c 
### What does this PR try to resolve?

This was the original behavior.
There was some concern over this previously and we switched to only
loading config from CARGO_HOME in #14749.
After discussing this in today's cargo team meeting, we decided to
switch it back.

A concern brought up previously was if you previously downloaded a
config and now download and run a script, you could get surprising
behavior, maybe even dangerous.
This does require some extra hoops because you don't have a
`.cargo.toml` but a `.cargo/config.toml`.
You can't directly download that but must first download a zip file and
then decompress it without it having a parent directory.

Contrast that with users who have a script in their repo and config that
should apply to it.
This is an important use case but one we will get less feedback on
during calls for testing.

In discussing this, we felt there are different use cases:
- The repo with a config file
- People surprised at how config loading works

We settled on this change because it is consistent with the current
behavior, even if it can be confusing and undesirable. We can then work
to improve the overall config search path experience and both regular
cargo commands and running of cargo scripts would benefit.

### How to test and review this PR?

This reverts commit bd47da1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@epage epage Awaiting requested review from epage

Assignees

@epage epage

Labels

A-cli Area: Command-line interface, option parsing, etc. Command-run 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.84.0

Development

Successfully merging this pull request may close these issues.

4 participants

@0xPoe @epage @bors @rustbot