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

Allow overriding vendored sources under sudo #40791

Closed
wants to merge 1 commit into from
Closed

Allow overriding vendored sources under sudo #40791

wants to merge 1 commit into from

Conversation

Slabity
Copy link

@Slabity Slabity commented Mar 24, 2017

When configuring rust to be built, the user can run configure with the option --disable-vendor or put vendor = false in config.toml

By default, when running the build system under sudo, the build system detects this and uses vendored crates to preserve the user's $HOME. This overrides any explicit options.

I believe that if a user is building under sudo and explicitly disables vendored sources, then the build system should respect that regardless. This patch allows for that.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Thanks for the PR! Unfortunately though this is just a bug in the build system, and I don't think that this is the best way to tackle it. The rationale behind forcing usage of vendor sources is that sudo can cause Cargo to spray root-owned files in $HOME otherwise, which is unlikely to be what you want.

This was a bug introduced in #39728 where the assumption that the vendor dir is always populated is no longer true.

Unfortunately I don't know the best way to fix this bug.

@Slabity
Copy link
Author

Slabity commented Mar 24, 2017

The rationale behind forcing usage of vendor sources is that sudo can cause Cargo to spray root-owned files in $HOME otherwise, which is unlikely to be what you want.

Could you explain this a bit more? The sudo command defaults to changing the environment variables to the default for the user you are running the command as unless explicitly use the -E option to preserve your environment. Running sudo env will result in HOME=/root.

The sudo command is also not exclusively used to become root. In the portage build system, we switch to a dedicated portage user and explicitly set the $HOME variable to a clean location that isn't touched by anything else.

I don't think it's unreasonable to allow a user to bypass vendor sources under sudo as long as it's done explicitly.

@alexcrichton
Copy link
Member

Unfortunately it looks like the behavior of sudo may differ across platforms, when I run it locally I get HOME=/home/alex. In that sense I don't think we're safe from the main concern, which is Cargo creating root-owned files in the user's home directory.

@Slabity
Copy link
Author

Slabity commented Mar 27, 2017

Unfortunately it looks like the behavior of sudo may differ across platforms, when I run it locally I get HOME=/home/alex

That's interesting. I never knew that.

I'm struggling to think of a better solution though. Perhaps a new flag like --disable-vendor-force? That way it protects normal users that may make that mistake and does not handhold those who do not need it.

@alexcrichton
Copy link
Member

I'd be ok with a solution that looks like:

  • Don't auto-enable vendoring if the vendor dir doesn't exist
  • If the vendor dir doesn't exist and we detect sudo, print a warning

@alexcrichton
Copy link
Member

ping @Slabity, any updates here?

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017
@arielb1
Copy link
Contributor

arielb1 commented Apr 18, 2017

Hi @Slabity. We haven't been hearing from you for some time 🕸. I'll close the PR to keep our queue clean, but feel free to reopen it if you are still interested in this PR.

@arielb1 arielb1 closed this Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants