rubygems / bundler

Manage your Ruby application's gem dependencies
https://bundler.io
MIT License
4.88k stars 1.99k forks source link

Don't spawn a shell when git pushing on release #7510

Closed deivid-rodriguez closed 4 years ago

deivid-rodriguez commented 4 years ago

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

The problem was that I almost went crazy when trying to release the last bundler version. rake release was hanging and I didn't know why.

What was your diagnosis of the problem?

Thanks to passing DEBUG=true to the task, I noticed that it was git push and git push --tags commands that were hanging:

$ DEBUG=true bin/rake release
["gem", "build", "-V", "/home/deivid/Code/bundler/bundler.gemspec"]
bundler 2.1.2 built to pkg/bundler-2.1.2.gem.
["git", "diff", "--exit-code"]
["git", "diff-index", "--quiet", "--cached", "HEAD"]
["git", "tag"]
["git", "tag", "-m", "Version 2.1.2", "v2.1.2"]
Tagged v2.1.2.
git push git push  --tags
^Crake aborted!
Interrupt: 
/home/deivid/Code/bundler/lib/bundler/gem_helper.rb:198:in `read'
/home/deivid/Code/bundler/lib/bundler/gem_helper.rb:198:in `popen'
/home/deivid/Code/bundler/lib/bundler/gem_helper.rb:198:in `block in sh_with_status'
/home/deivid/Code/bundler/lib/bundler/shared_helpers.rb:52:in `chdir'
/home/deivid/Code/bundler/lib/bundler/shared_helpers.rb:52:in `block in chdir'
/home/deivid/Code/bundler/lib/bundler/shared_helpers.rb:51:in `chdir'
/home/deivid/Code/bundler/lib/bundler/gem_helper.rb:197:in `sh_with_status'
/home/deivid/Code/bundler/lib/bundler/gem_helper.rb:133:in `perform_git_push'
/home/deivid/Code/bundler/lib/bundler/gem_helper.rb:115:in `git_push'
/home/deivid/Code/bundler/lib/bundler/gem_helper.rb:64:in `block (2 levels) in install'
/home/deivid/Code/bundler/lib/bundler/gem_helper.rb:160:in `tag_version'
/home/deivid/Code/bundler/lib/bundler/gem_helper.rb:64:in `block in install'
/home/deivid/Code/bundler/Rakefile:14:in `block in invoke'
/home/deivid/Code/bundler/Rakefile:13:in `invoke'
/home/deivid/Code/bundler/spec/support/rubygems_ext.rb:87:in `load'
/home/deivid/Code/bundler/spec/support/rubygems_ext.rb:87:in `gem_load_and_activate'
/home/deivid/Code/bundler/spec/support/rubygems_ext.rb:45:in `gem_load'
Tasks: TOP => release => release:source_control_push
(See full trace by running task with --trace)

The debugging output is very interesting because it also tells us that these commands are the only ones passing Strings to IO.popen instead of Arrays. That means these commands are spawning a new shell.

That's when I realized that I have hub installed on my system and git aliased to it. So I figure this aliasing was interacting with the subcommand in a bad way.

Indeed, disabling hub fixed the issue and let me make the release.

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

I think we can avoid other issues similar to this for people pushing releases by not spawing a shell here, just like we do with all of the other commands.

I think it's a good practice anyways.

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

I chose this fix because I think it makes the code better. Of course I can disable hub every time I release, but I think this is worth doing.

simi commented 4 years ago

In theory, is it possible to break someone's "flow" by this change? If so, did that "flow" worked just by accident? It probably heavily relies on each local setup.

Anyway this change seems reasonable :+1: .

deivid-rodriguez commented 4 years ago

I can't image any "flow" that this change would break... :thinking:. I thought of a potential scenario where git is only made available to the user (put in the PATH) by the .bashrc file or something. But, our gem helpers already run many other git commands without spawning new shells, so nothing would really work in that case.

deivid-rodriguez commented 4 years ago

@bundlerbot merge

ghost commented 4 years ago

Build succeeded