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

Modify shebang to use settings #6488

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cdenneen
Copy link

Fixes #6478

@welcome
Copy link

welcome bot commented Mar 15, 2023

Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems 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 GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions 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 #rubygems or #bundler channel on Slack.

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

@@ -172,7 +172,7 @@ def install(spec, options = {})
:bin_dir => bin_path.to_s,
:ignore_dependencies => true,
:wrappers => true,
:env_shebang => true,
:env_shebang => Bundler.settings[:shebang],
Copy link
Member

Choose a reason for hiding this comment

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

🤔 I see there was some discussion for this at #6478 already. Looking at this again, Bundler.settings[:shebang] is nil by default. To keep this backward compatible, would it make sense to keep the default true, but respect Bundler.settings[:shebang] when set (aka when not nil)? Also some test would be great to cover this.

@simi
Copy link
Member

simi commented Mar 22, 2023

If I understand it well, this current code change changes the behaviour, but there are no failing specs. Would it be possible to cover this better in test suite?

@cdenneen
Copy link
Author

@simi I created this PR based on @deivid-rodriguez suggestion. I wouldn't know how to do the requested changes. Please feel free to update the PR or replace with new one that includes this fix.

Thanks

@deivid-rodriguez
Copy link
Member

@cdenneen Can you confirm that you have --no-env-shebang in your ~/.gemrc. After a second thought, I think the right fix for this is to check that RubyGems setting.

Unifying Bundler & RubyGems shebang behavior would be a separate thing.

@cdenneen
Copy link
Author

cdenneen commented Jun 2, 2023

@cdenneen Can you confirm that you have --no-env-shebang in your ~/.gemrc. After a second thought, I think the right fix for this is to check that RubyGems setting.

Unifying Bundler & RubyGems shebang behavior would be a separate thing.

I'm not putting that in a .gemrc as these are builds on multiple windows machines being built with packer.
I can have packer create a .gemrc but think the issue is it's behavior being different is more of the issue.
I can't see any reason why a gem install and a bundle install of a gem creates binstubs but one has env-shebang and the other doesn't.

How do you propose to check the RubyGems setting to fix this?

So your suggestion is to create a .gemrc that specifies:

---
install: --no-document --no-env-shebang
update: --no-document --no-env-shebang

@cdenneen
Copy link
Author

cdenneen commented Jun 2, 2023

@cdenneen Can you confirm that you have --no-env-shebang in your ~/.gemrc. After a second thought, I think the right fix for this is to check that RubyGems setting.

Unifying Bundler & RubyGems shebang behavior would be a separate thing.

@deivid-rodriguez also it's been a bit since I opened this but if I recall the issue here isn't the gem install but the bundle install. gem install produces the correct shebang on the binstubs whereas bundler was putting in the env-shebang.
I ended up having to stop using Bundler in our packer builds because of this but it's lead to issues where I can't scale --jobs 4 during install. I also can't lock with Gemfile.lock all the dependencies that get installed. It's really been frustrating not being able to leverage Bundler for these builds to ensure consistency vs getting an updated dependency that now might break something. I agree with @simi there isn't a test around this but feel it's useful change vs forced to true as it is now.

So doesn't Bundler.settings[:shebang] currently defaultsto true so this change doesn't actually affect anything unless you specify Bundler.settings[:shebang] to be false or something?

@cdenneen
Copy link
Author

cdenneen commented Jun 2, 2023

I've tried adding this gemrc file to C:\Users\Administrator\.gemrc,C:\Users\Administrator\gemrc, C:\ProgramData\gemrc and bundle install still results in:

C:\Program Files\Puppet Labs\Puppet\puppet\bin>more r10k
#! ruby

Where as when I did the gem install

C:\Program Files\Puppet Labs\Puppet\puppet\bin>more r10k
#!"C:/Program Files/Puppet Labs/Puppet/puppet/bin/ruby.exe"

@cdenneen
Copy link
Author

cdenneen commented Jun 2, 2023

This will be bigger issue for me since the embedded ruby is 2.5 so latest ruby gem I can upgrade to is the 3.3.x releases, would need to patch this into 3.3.27 or something

@deivid-rodriguez
Copy link
Member

Hei!

So what I meant to say is that I realized that the shebang Bundler setting is meant to control the project binstubs bundle binstubs generates, not global binstubs generated by RubyGems.

So, instead of respecting that setting here, I think we should respect the RubyGems setting.

That RubyGems setting is controlled by the --[no-]env-shebang RubyGems flag, and defaults to false.

Regardless of this, handling backwards compatibility seems hard here 🤔.

@cdenneen
Copy link
Author

@deivid-rodriguez do you know of any workaround/hack for this?
I'd like to do the install with a Gemfile/Gemfile.lock but just need the shebang line for all the resulting binaries to be #!/PATH/TO/EMBEDDED/ruby instead of #!env ruby since that seems to break constantly on my Windows setup.

If I install the gems via gem install the shebang is fine but I keep running into dependency update issues breaking on me so if I can lock down with Gemfile.lock and can also leverage the bundle install --jobs 4 parallel option, it would make things easier.

@deivid-rodriguez
Copy link
Member

Given the value is currently hardcoded, there's no workaround at the moment.

Regarding backwards compatibility, it should be very rare for someone to want conflicting settings for Bundler & RubyGems (if we go with using the value of --env-shebang configured at ~/.gemrc), or conflicting settings for project and global binstubs (if we go with Bundler.settings[:shebang]).

So I think I'd go with @simi's suggestion at #6488 (comment).

If we go that way, we need to:

  • Make this PR more compatible by implementing what @simi suggested.
  • Add a spec for this feature.
  • Update shebang setting documentation to mention that it applies to any shebang generated by Bundler (for both project binstubs and global binstubs).

Allow shebang to use settings
@zacheryph
Copy link

settings[:shebang] and env-shebang are related, but are not the same at all.

The shebang setting is a string, where as env-shebang is a boolean.

Adding env-shebang as a flag would require addition of handling that flag itself, as well as documentation.

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.

bundler install shebang
4 participants