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

Support bundler local_search on 2.4.1 and higher #597

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

JoshReedSchramm
Copy link
Contributor

@JoshReedSchramm JoshReedSchramm commented Dec 29, 2022

Description

This updates the call to local_search in bundler for the new query format on version 2.4.1 or higher.

Fixes #596

@JoshReedSchramm
Copy link
Contributor Author

A couple notes

  • I tried to maintain support for old and new versions of bundler but if there's a better way to do the version comparison I'd love the feedback.
  • Given that this is for the version of bundler itself I wasn't sure how to add a test to the bundler tests for his scenario.
  • Also I ran into issues trying to run tests locally even before I made this change, even after following the steps in the contributor guide.

Copy link
Contributor

@jonabc jonabc left a comment

Choose a reason for hiding this comment

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

thank you so much for opening this PR! I left two suggested changes, with the suggestion for changing the version check being more critical than the other.

As a small style nit, because the change in logic between the two versions is so limited, it would be nice to have the change encapsulated to a separate method. If you'd prefer not to handle that in this PR that's totally fine, I can update it as a follow up. I'm thinking of something like

def bundler_query
  if Gem::Version.new(::Bundler::VERSION) >= Gem::Version.new("2.4.0")
    ["bundler", ::Bundler.gem_version]
  else
    Gem::Dependency.new("bundler", ::Bundler::VERSION)
  end
end

lib/licensed/sources/bundler/definition.rb Outdated Show resolved Hide resolved
lib/licensed/sources/bundler/definition.rb Outdated Show resolved Hide resolved
@jonabc
Copy link
Contributor

jonabc commented Dec 30, 2022

  • Given that this is for the version of bundler itself I wasn't sure how to add a test to the bundler tests for his scenario.

Yep agreed. The way to test this is to add '2.4' to the list of tested bundler versions in CI . If that passes with the changes in this PR then all good!

  • Also I ran into issues trying to run tests locally even before I made this change, even after following the steps in the contributor guide.

Can you describe what issues you ran into? Assuming they're related to repository setup and not specific to using bundler >= 2.4.0 which is fixed by this PR, I'd love to get those fixed.

@jonabc
Copy link
Contributor

jonabc commented Dec 30, 2022

oh and as one final comment, don't worry about the failing license metadata CI check. It's an issue with using a repo secret in a PR opened from a fork. I still haven't figured out a good way to get that working, and it's not a required check 🤷

@jonabc
Copy link
Contributor

jonabc commented Jan 3, 2023

🚀 thanks so much for fixing this. FYI the next release, which will include this fix, will bump the major version due to containing some breaking changes in the release process outside of my control.

@jonabc jonabc merged commit edde586 into github:master Jan 3, 2023
jonabc added a commit that referenced this pull request Jan 5, 2023
### Added
- Licensed supports Cocoapods as a dependency source (:tada: @LouisBoudreau #584)
- Licensed supports Gradle multi-project builds (:tada: @LouisBoudreau #583)
### Fixed
- Licensed no longer crashes when run with Bundler >= 2.4.0 (:tada: @JoshReedSchramm #597)
### Changed
- BREAKING: Licensed no longer ships executables with releases (#586)
- BREAKING: Licensed no longer includes support for Go <= 1.11
@jonabc jonabc mentioned this pull request Jan 5, 2023
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.

Licensed breaks under Bundler 2.4.1 due to dependency search change
2 participants