Skip to content

Add safe-directories. #3005

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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.

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

Successfully merging this pull request may close these issues.

4 participants