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

Add --all-platforms flag to bundle binstubs to generate binstubs for all platforms #3886

Merged

Conversation

SeekingMeaning
Copy link
Contributor

@SeekingMeaning SeekingMeaning commented Aug 14, 2020

Description:

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

This closes #2120

I stumbled across the fact that when working with jruby on a Linux system, no Windows stub files are generated for installed gems. This is reasonable for a native ruby installation, but it would be nice to also generate windows stubs in a jruby environment because this would make the jruby folder portable also across OS types as long as there is a JRE available.

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

This adds --all-platforms option suggested by segiddins

I think the manpages also should be updated but I'm not quite sure how to go about doing that


Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

@welcome
Copy link

welcome bot commented Aug 14, 2020

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

@SeekingMeaning SeekingMeaning force-pushed the all-platform-binstubs branch 2 times, most recently from 3b4e9f5 to 8b1f655 Compare August 15, 2020 23:08
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Hi @SeekingMeaning! I didn't review this because I thought it needed work due to some specs being broken. But now I looked at the failing specs and I'm not 100% sure they failures are related to this PR. They seem to break only on Windows, so that might indicate a relationship, but still I'm not fully sure.

Could you rebase and force-push this PR just to make sure?

@deivid-rodriguez
Copy link
Member

And thanks for this, by the way! 😃

@SeekingMeaning SeekingMeaning force-pushed the all-platform-binstubs branch 3 times, most recently from 83222fd to 03c21c4 Compare November 4, 2020 21:37
@deivid-rodriguez
Copy link
Member

@SeekingMeaning This looks good, but could you add some tests for it?

@SeekingMeaning
Copy link
Contributor Author

Would adding this to binstubs_spec.rb work?

    it "allows installing binstubs for all platforms" do
      install_gemfile <<-G
        source "#{file_uri_for(gem_repo1)}"
        gem "rack"
      G

      bundle "binstubs rack --all-platforms"

      expect(bundled_app("bin/rackup")).to exist
      expect(bundled_app("bin/rackup.cmd")).to exist
    end

@deivid-rodriguez
Copy link
Member

That seems enough to me, yeah 👍. But since there's two different places where we're adding the check for the new option to generate the extra binstubs, I'd add a separate spec that covers the remaining place (I think you need to also run bundle binstubs --standalone --all-platforms, or something like that).

@deivid-rodriguez
Copy link
Member

@SeekingMeaning This looks great to me, thanks! Could you rebase on top of latest master and squash both commits into one?

@deivid-rodriguez deivid-rodriguez changed the title Add option to install binstubs for all platforms Add --all-platforms flag to bundle binstubs to generate binstubs for all platforms Nov 12, 2020
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks so much @SeekingMeaning!! 💜

@deivid-rodriguez deivid-rodriguez merged commit ba09c54 into rubygems:master Nov 12, 2020
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Add `--all-platforms` flag to `bundle binstubs` to generate binstubs for all platforms

(cherry picked from commit ba09c54)
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Add `--all-platforms` flag to `bundle binstubs` to generate binstubs for all platforms

(cherry picked from commit ba09c54)
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Add `--all-platforms` flag to `bundle binstubs` to generate binstubs for all platforms

(cherry picked from commit ba09c54)
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Add `--all-platforms` flag to `bundle binstubs` to generate binstubs for all platforms

(cherry picked from commit ba09c54)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Give option to generate Windows bin stubs regardless of host OS
3 participants