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

Source bundler dependencies without using ::Bundler::Definition#specs #54

Merged
merged 13 commits into from
Jun 22, 2018

Conversation

jonabc
Copy link
Contributor

@jonabc jonabc commented Jun 15, 2018

Fixes #53

This PR updates the bundler source to support finding dependencies in the Bundler source without using Bundler's internal specification materialize logic. It largely aims for unchanged function, though there are two changes called out below.

Why

In aiming to be able to distribute a packaged executable of ruby+licensed, there were issues with the bundler source. The executable uses a squashed file system containing ruby, licensed and bundler. When licensed evaluates a source using the bundler object model, it's using the bundler gem available inside the squashed file system rather than the bundler gem on the host.

When using the bundler gem from the squashed file system, it uses gem information also from inside the squashed file system because it doesn't know about the host file system. There is a fundamental mismatch which makes sense given that we're running bundler from a separate file system and expecting it to find information from the host automagically.

How

Custom logic is added to the bundler source to enumerate specifications that is overall simpler and more straightforward to read through and debug despite having more custom logic in the source.

licensed requires that bundle install was already successfully run for dependencies to be enumerated. As a result, we know that all (*) specifications can be found at ::Bundle.specs_path. Both Gem::Specification and ::Bundler::Specification support #full_name so we can find the specification using ::Bundle.specs_path.join("#{spec.full_name}.gemspec").

(*) - There are two exceptions

  1. The bundler gemspec itself. Even when bundler is an explicit Gemfile dependency, it still does not put it's own gemspec under ::Bundler.specs_path. More on this in a bit
  2. Gemfile dependencies from local gemspecs. In this scenario we actually don't need to do much. The realized specification can be obtained from the un-realized specification through spec.source.specs.first, where source represents the gemspec itself.

It's worth calling out that obtaining the bundler Gem::Specification is special cased. To support the self-contained executable scenario from #50, licensed can't use the currently loaded ::Bundler gem API because it references the gem from the self-contained executable. It has no knowledge of the host machines Bundler state or installed location. Instead licensed runs bundler as a separate process on the host machine to find the bundler gem path which is then used to find the specification.

Functional changes

  1. Bundler is no longer included as a dependency unless it's explicitly called out in the Gemfile. If the gem hasn't been specified in the Gemfile it's considered a development dependency and filtered.

    • /cc @mlinksva Is this 👍 considering it's likely that bundler is generally assumed to be on the machine and doesn't need to be listed in the Gemfile even when it's a runtime dependency.
  2. Configuration overrides for groups to exclude from dependencies is allowed. Previous functionality is maintained with the default values :development and :test.

@mlinksva
Copy link
Contributor

@jonabc I think that's fine. The thing that would make it not fine (IIUC) would be if there are licensed users who have bundler as a non-explicit but actual runtime dependency and therefore ought be checking/documenting its license but wouldn't be with this change. Assuming that bundler is installed without specifying it seems like a bad idea that would eventually bite such a user anyway, thus an edge case that we shouldn't worry about. Sound right?

Also, I looked at the changes here and don't understand well enough to see why without this PR, licensed includes bundler as a dependency even if it is not specified. Just out of curiosity, where is this PR filtering out bundler unless it is explicitly included?

don't ever include the development dependencies of non-local gems
sometimes bundler doesn't know where bundler is
# Don't return gems added from `add_development_dependency` in a gemspec
# if the :development group is excluded
gemspec_source = source.is_a?(::Bundler::Source::Gemspec)
return false if dependency.type == :development && (!gemspec_source || exclude_development_dependencies?)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This filters out gems that are added to a gemspec via add_development_dependency. !gemspec_source covers dependencies that are found recursively from other dependencies (source should be nil). exclude_development_dependencies? checks that the :development group is excluded via bundler or licensed configuration.

If bundler was listed as a development dependency via a local gemspec, or if it was found recursively as a development dependency of a runtime dependency, it will be filtered here.

return true if dependency.groups.nil?

# check if the dependency is in any groups we're interested in
(dependency.groups & groups).any?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This filters our gems that are not found in any Gemfile groups that will be included in licensed actions.

If bundler is listed as a dependency under a :development group, it'll get filtered here when the :development group is excluded either by bundler or licensed configurations.

By default licensed does exclude the :development group.

@jonabc
Copy link
Contributor Author

jonabc commented Jun 18, 2018

The thing that would make it not fine (IIUC) would be if there are licensed users who have bundler as a non-explicit but actual runtime dependency and therefore ought be checking/documenting its license but wouldn't be with this change.

Yea, that situation is what I'm concerned about as well.

Assuming that bundler is installed without specifying it seems like a bad idea that would eventually bite such a user anyway, thus an edge case that we shouldn't worry about.

👍 Agreed that assuming anything is installed without specifying it is a bad idea. My concern is that I don't know if it's edge case or if it's common. Bundler is the de facto package manager for ruby and is installed almost everywhere - it seems pretty easy to assume that it's going to be available at runtime.

Also, I looked at the changes here and don't understand well enough to see why without this PR, licensed includes bundler as a dependency even if it is not specified.

I've looked through the bundler source a few times and I'm still not sure I understand it. I think bundler might always add itself with type == :development if not otherwise specified in a Gemfile or gemspec.

Just out of curiosity, where is this PR filtering out bundler unless it is explicitly included?

I added some review comments trying to explain a little better how dependencies get filtered out, with specific examples of how bundler might be filtered when it's a development dependency.

@jonabc
Copy link
Contributor Author

jonabc commented Jun 18, 2018

As CI highlighted, bundler doesn't always know where bundler is located.

I switched to using gem specification and Gem::Specification.from_yaml to create the bundler specification. This should be safe and reliable - bundler should be used from the current gem list that is also used for gem specification.

It looks like CI is green with that change

@jonabc
Copy link
Contributor Author

jonabc commented Jun 18, 2018

And finally, to highlight the reason behind the PR. With these changes I'm able to build executables for licensed that can be used for all dependency sources, in any environment, whether ruby is present or not 🎉 🎉

➜  ./licensed list -c test/fixtures/config/.licensed.yml
Displaying dependencies for licensed-yml
  rubygem dependencies:
    Found addressable (2.5.2)
    Found bundler (1.16.1)
    Found dotenv (2.4.0)
    Found faraday (0.15.2)
    Found licensee (9.9.1)
    Found multipart-post (2.0.0)
    Found octokit (4.8.0)
    Found pathname-common_prefix (0.0.1)
    Found public_suffix (3.0.2)
    Found rugged (0.27.1)
    Found sawyer (0.8.1)
    Found thor (0.20.0)
    Found tomlrb (1.2.7)
  * rubygem dependencies: 13

@tenderlove
Copy link

Assuming that bundler is installed without specifying it seems like a bad idea that would eventually bite such a user anyway, thus an edge case that we shouldn't worry about. Sound right?

I agree with this assessment. In order to gem install such a gem, there would have to be something in the dependency tree that depends on it (otherwise you just wouldn't be able to install that gem). Today, the only things that can be implicitly depended upon and not installed are things that ship with Ruby's standard library.

@jonabc
Copy link
Contributor Author

jonabc commented Jun 20, 2018

@benbalter can you take a look at this? I'd ❤️ to be able to attach executable packages along with each licensed release.

@jonabc
Copy link
Contributor Author

jonabc commented Jun 22, 2018

Merging. Can address any issues if/as they come up.

@jonabc jonabc merged commit 511d771 into master Jun 22, 2018
@jonabc jonabc deleted the bundler-materialize-agnostic branch June 22, 2018 18:10
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.

None yet

3 participants