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

Add safe-directories. #3005

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add safe-directories. #3005

wants to merge 6 commits into from

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jun 12, 2022

This is an implementation of RFC 3279.

@ehuss
Copy link
Contributor Author

ehuss commented Jun 12, 2022

Questions/Notes:

  • I did not add any sort of "unstable" flag to enable this feature. Which approach do you think would be good?

    • Merge this on a separate branch, and create a testing release from that?
    • Merge on master, and revert it if a release needs to be made before we commit to this feature?
    • Add some unstable gate, such as a CLI flag or environment variable like RUSTUP_UNSTABLE_SAFE_DIRECTORIES=true to mirror the Cargo one.
      This might be the most desirable, but requires changing a bunch of things to support it.
  • Should I bump DEFAULT_METADATA_VERSION? I think the only consequence of not doing that is that an older version of rustup may delete the safe_directories settings. If it is bumped, then older rustups will just fail, which seems worse.

  • I can't tell why some code uses macros like info! and others use a Notification.

  • I can't tell why some code is in rustup_mode.rs and some is in common.rs.

  • The ownership checking code is copied from Cargo until a new cargo_util is published.

@kinnison
Copy link
Contributor

How did the cargo part of this go Eric? I'm reluctant to start validating this until we know Cargo makes sense.

@ehuss
Copy link
Contributor Author

ehuss commented Aug 30, 2022

I need to follow up on some things, and I have been procrastinating. 🐌

@rbtcollins
Copy link
Contributor

Perhaps rustup and cargo should share their safe directories flag? Would we ever want them to disagree?

@ehuss
Copy link
Contributor Author

ehuss commented Feb 19, 2023

I'm not sure I understand the question fully. Can you say more about what you mean?

If it is about why there is a separate CARGO_SAFE_DIRECTORIES and RUSTUP_SAFE_DIRECTORIES, the reason is because cargo can be used without rustup (and it would be a little strange to set a RUSTUP_ setting without rustup). However, if you are using rustup, then the intent is that the user should only configure RUSTUP_SAFE_DIRECTORIES (because cargo will read that) so in essence there is only one setting for most users.

I suppose it may be possible to drop RUSTUP_SAFE_DIRECTORIES and have rustup read CARGO_SAFE_DIRECTORIES, but that feels potentially confusing.

@rbtcollins
Copy link
Contributor

I'm hoping to entirely remove relative-path toolchain directories: I've asked around a bit and so far found no use case for them.

That then leaves absolute path toolchain directories - this seems safe to me; would welcome thoughts on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants