Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Display reason to require sudo #6316

Merged
8 commits merged into from Sep 24, 2018
Merged

Display reason to require sudo #6316

8 commits merged into from Sep 24, 2018

Conversation

okkez
Copy link
Contributor

@okkez okkez commented Mar 1, 2018

This is useful for non-interactive installation with bundler.

What was the end-user problem that led to this PR?

treasure-data/omnibus-td-agent#166

I could not notice that bundler needs sudo privilege from logs.
So I checked bundler code.

What was your diagnosis of the problem?

Bundler does not show the reason to need sudo privilege.

What is your fix for the problem, implemented in this PR?

Display reason to require sudo.

Why did you choose this fix out of the possible options?

If bundler displays reason to require sudo, we can notice permission problems as soon as possible.

@ghost
Copy link

ghost commented Mar 1, 2018

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

@okkez
Copy link
Contributor Author

okkez commented Mar 1, 2018

For example, a daemon program that has gem based plugin system.
We can manage plugins via Gemfile and the daemon installs plugins using bundler on boot.
If Gemfile is not writable by daemon user, bundler needs sudo and nothing to say in previous version.

@colby-swandale
Copy link
Member

Thanks for opening a pull request but this will require a test to confirm the behavior of this change before we will accept it.

@colby-swandale
Copy link
Member

Can you also please fill-in in the PR Template in the description.

@okkez
Copy link
Contributor Author

okkez commented Mar 1, 2018

I've filled PR template.

@okkez
Copy link
Contributor Author

okkez commented Mar 1, 2018

I could not find Bundler.requires_sudo? related spec under spec directory.
Could show me sample(pseudo) code to add test code for this PR?

lib/bundler.rb Outdated
@@ -367,6 +367,10 @@ def requires_sudo?
# if any directory is not writable, we need sudo
files = [path, bin_dir] | Dir[path.join("build_info/*").to_s] | Dir[path.join("*").to_s]
sudo_needed = files.any? {|f| !File.writable?(f) }
if sudo_needed
Copy link
Member

Choose a reason for hiding this comment

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

unwritable_files = files.reject {|f| File.writable?(f) }
sudo_needed = !unwritable_files.empty?
if sudo_needed
  ...

@segiddins
Copy link
Member

👍 with a test

@okkez
Copy link
Contributor Author

okkez commented Mar 6, 2018

I've added test for Bundler.requires_sudo?, though I'm not familiar with RSpec.

context "unwritable paths" do
before do
FileUtils.touch("tmp/vendor/bundle/unwritable.txt")
FileUtils.chmod(0400, "tmp/vendor/bundle/unwritable.txt")
Copy link
Member

Choose a reason for hiding this comment

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

I think rubocop wants you to change this to 0o400.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed CI result.

@segiddins
Copy link
Member

Thanks! Looks like this is failing on 1.8.7, but is otherwise good to go! https://travis-ci.org/bundler/bundler/jobs/350147434#L1020

@okkez
Copy link
Contributor Author

okkez commented Mar 12, 2018

@segiddins Do I have to fix spec for Ruby 1.8.7?

@colby-swandale
Copy link
Member

Yes, Bundler still supports ruby 1.8.7

@hsbt
Copy link
Member

hsbt commented Mar 12, 2018

Yes, Bundler still supports ruby 1.8.7

It's a doubt. Current master is not supported with Ruby 1.8.7. It only helps backport work. So, It's our workflow issue.

@colby-swandale
Copy link
Member

As this is not specifically for 2.0 this PR will probably be merged with 1.16. And i prefer we make sure this PR works with 1.8.7 now rather then when i try to make a release.

lib/bundler.rb Outdated
unwritable_files = files.reject {|f| File.writable?(f) }
sudo_needed = !unwritable_files.empty?
if sudo_needed
Bundler.ui.warn "Following files may not be writable, so sudo is needed: #{unwritable_files.map(&:to_s).join(",")}"
Copy link
Member

Choose a reason for hiding this comment

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

should this list be printed on multiple lines instead of being separated by ,. I imagine this list could in circumstances be quiet big and having it all printed on a single line wouldn't look very nice to the user.

@bundlerbot
Copy link
Collaborator

☔ The latest upstream changes (presumably #6318) made this pull request unmergeable. Please resolve the merge conflicts.

This is useful for non-interactive installation with bundler.
    lib/bundler.rb:372:120: C: Style/StringLiteralsInInterpolation: Prefer double-quoted strings inside interpolations.
              Bundler.ui.warn "Following files may not be writable, so sudo is needed: #{unwritable_files.map(&:to_s).join(',')}"
                                                                                                                           ^^^
In normal case, all elements in `files` are writable.
So all elements in `files` are scanned.
    spec/bundler/bundler_spec.rb:251:25: C: Style/NumericLiteralPrefix: Use 0o for octal literals.
            FileUtils.chmod(0400, "tmp/vendor/bundle/unwritable.txt")
                            ^^^^
Because Dir#[] will return unsorted results.
@colby-swandale colby-swandale added this to the 1.17.0 milestone Jun 6, 2018
@deivid-rodriguez
Copy link
Member

I think this is ready and with all feedback addressed.

@segiddins
Copy link
Member

@bundlerbot r+

@bundlerbot
Copy link
Collaborator

📌 Commit 025110c has been approved by segiddins

@bundlerbot
Copy link
Collaborator

⌛ Testing commit 025110c with merge 7882798...

bundlerbot added a commit that referenced this pull request Sep 10, 2018
Display reason to require sudo

This is useful for non-interactive installation with bundler.

### What was the end-user problem that led to this PR?

treasure-data/omnibus-td-agent#166

I could not notice that bundler needs sudo privilege from logs.
So I checked bundler code.

### What was your diagnosis of the problem?

Bundler does not show the reason to need sudo privilege.

### What is your fix for the problem, implemented in this PR?

Display reason to require sudo.

### Why did you choose this fix out of the possible options?

If bundler displays reason to require sudo, we can notice permission problems as soon as possible.
@bundlerbot
Copy link
Collaborator

💥 Test timed out

@segiddins
Copy link
Member

@bundlerbot retry

1 similar comment
@colby-swandale
Copy link
Member

@bundlerbot retry

@colby-swandale
Copy link
Member

@bundlerbot r+

@colby-swandale
Copy link
Member

@bundlerbot r=colby-swandale

@colby-swandale
Copy link
Member

@bundlerbot r+

@ghost
Copy link

ghost commented Sep 24, 2018

Not awaiting review

ghost pushed a commit that referenced this pull request Sep 24, 2018
6316: Display reason to require sudo r=colby-swandale a=okkez

This is useful for non-interactive installation with bundler.

### What was the end-user problem that led to this PR?

treasure-data/omnibus-td-agent#166

I could not notice that bundler needs sudo privilege from logs.
So I checked bundler code.

### What was your diagnosis of the problem?

Bundler does not show the reason to need sudo privilege.

### What is your fix for the problem, implemented in this PR?

Display reason to require sudo.

### Why did you choose this fix out of the possible options?

If bundler displays reason to require sudo, we can notice permission problems as soon as possible.


Co-authored-by: Kenji Okimoto <okimoto@clear-code.com>
@ghost
Copy link

ghost commented Sep 24, 2018

Build succeeded

@ghost ghost merged commit 025110c into rubygems:master Sep 24, 2018
colby-swandale pushed a commit that referenced this pull request Oct 9, 2018
6316: Display reason to require sudo r=colby-swandale a=okkez

This is useful for non-interactive installation with bundler.

### What was the end-user problem that led to this PR?

treasure-data/omnibus-td-agent#166

I could not notice that bundler needs sudo privilege from logs.
So I checked bundler code.

### What was your diagnosis of the problem?

Bundler does not show the reason to need sudo privilege.

### What is your fix for the problem, implemented in this PR?

Display reason to require sudo.

### Why did you choose this fix out of the possible options?

If bundler displays reason to require sudo, we can notice permission problems as soon as possible.


Co-authored-by: Kenji Okimoto <okimoto@clear-code.com>
(cherry picked from commit 1bd53e3)
colby-swandale added a commit that referenced this pull request Oct 25, 2018
* 1-17-stable: (38 commits)
  Version 1.17.0 with changelog
  Merge #6754
  Version 1.17.0.pre.2 with changelog
  fix failing bundle remove specs
  Still document the `--force` option
  scope specs testing bundler 2 deprecations to bundler 1 only
  Merge #6718
  Merge #6707
  Merge #6702
  Merge #6316
  Auto merge of #6447 - agrim123:agr-update-error-message, r=segiddins
  Auto merge of #6513 - agrim123:agr-bundler-remove, r=indirect
  Auto merge of #6318 - jhawthorn:fix_incorrect_test_in_requires_sudo, r=segiddins
  Auto merge of #6450 - bundler:segiddins/bundle-binstubs-all, r=colby-swandale
  Auto merge of #6024 - gwerbin:custom-user-bundle-dir, r=colby-swandale
  Version 1.17.0.pre.1 with changelog
  Auto merge of #5964 - bundler:colby/deprecate-viz-command, r=segiddins
  Auto merge of #5986 - bundler:seg-jobs-count, r=indirect
  Auto merge of #5995 - bundler:seg-gvp-major, r=indirect
  Auto merge of #5803 - igorbozato:path_relative_to_pwd, r=indirect
  ...
@rubygems rubygems deleted a comment from nleo Jul 11, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants