Remove git subshelling from gemspec

[deivid-rodriguez]

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

The initial problem was that the default bundler gemspec integrated in ruby-core was shipped nearly empty: https://bugs.ruby-lang.org/issues/15582. That caused issues in bundler: #6937

What was your diagnosis of the problem?

I looked into ruby-core history, and still not fully sure how it happened but something I noticed is that bundler does not make this integration easy because ruby-core doesn't have a git environment available, so it needs to do a bit of juggling: https://github.com/ruby/ruby/blob/trunk/tool/rbinstall.rb#L802-L803. We could make things easier.

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

My fix is to stop subshelling to git from our own gemspec. I think we do this in the default generated gemspec so that newbie users don't accidentally ship generated files with their gems (although the current solution would still ship generated files that newbie users accidentally commit to source control 😄). But we shouldn't consider ourselves newbies, so I think we can avoid that.

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

I chose this fix because it avoids shelling out to git inside the gemspec while still keeping some assurances about the correctness of the shipped set of files through a quality spec.

[simi]

If I understand it well, specs already relies on git and this is not introducing new dependency for them.

👍

[deivid-rodriguez]

Yeah, our specs already depend on git.

The only problem with this PR is that for now I removed the possibility of reading the gemspec correctly from outside of the root folder of the gem, because the base_dir: parameter to Dir.glob only exists since ruby 2.5. I guess I could check the running ruby and use this solution only for the latest rubies, but I'm not sure if this is worth it.

I'd like @hsbt's opinions on whether something like this would make the ruby-core installer easier to implement, and less likely to run into issues.

[hsbt]

The ruby core collect files of bundler when installing the ruby binaries with https://github.com/ruby/ruby/blob/trunk/tool/rbinstall.rb#L661

This change is helpful to us. The ruby core needs to support a non-git environment like a docker.

[colby-swandale]

As the person that has been releasing Bundler, i'm not comfortable with this PR being merged. I heavily depend on git being used in the gemspec to know exactly what files are going to be shipped in a release.

Each release will often require some sort of change that i need to make. This ranges from updating the Changelog, to fixing broken tests, or just quality improvements. See the merge commits from the stable branches into master that i make every time a release is published. During this release process, Git provides a small but critical barrier from accidentally introducing files into a release. This obviously helps keep quality control for bundler (one of the most used ruby tools ever) up and helps ensure we don't ship broken releases.

I understand that ruby has had some issues with Bundler & Git but that should not be given priority over our own team's release flow.

[indirect]

Maybe there's a way to get both things? It makes sense to me to want to provide ruby-core with a way to package Bundler without git installed, but it also seems like an important and helpful release process thing to have git as a backstop against accidentally including the wrong files.

The quality spec that checks the gemspec against the git file list is a good step in the direction of providing some assurance. Maybe we could also add something to the gem build step that checks the files that are going to be released against the files that are checked in to git, and errors out listing the files that don't match if there are files that don't match?

I'd be happy to hear more thoughts from both of you about possible solutions. 👍

[deivid-rodriguez]

Yeah, by no means I want to break our own workflows, let's make this better for everyone.

I realize that the current quality spec is not super-helpful since the environment in CI will be usually clean. My idea was to introduce the same check I added to the specs, but as a prerrequisite for the :release task. How does that sound?

@indirect If I understand your idea right, you suggest moving this check to rubygems itself? That's interesting, I hadn't considered that... 🤔

[deivid-rodriguez]

I gave it a little more thought and I think making this validation on gem build is not going to work, since this behavior is not always what users want. For example, developers of fat binary gems will want generated artifacts to be included with the gem even if they're not on source control. Right?

I think the best place for this check would be in a custom prerequisite to rake release.

[indirect]

Sorry my explanation wasn’t clear! I also meant as a dependency of the rake task named :build 🙂

[deivid-rodriguez]

I added the release file check in 3470aef. This PR is not yet ready (I need to restore building the gemspec from folders different from the root folder), but I want to make sure first. @colby-swandale are you good with this given the checks we are adding?

[deivid-rodriguez]

Actually, I misunderstood what git -C was trying to do here. It was fixing #6029, which still works with this PR.

Building the current bundler.gemspec from a different directory to where the files are doesn't work with or without this PR, because git -C returns relative paths, which are interpreted by gem build as local to the cwd. Actually it does work with rubygems 3 due to ruby/rubygems#2204, but that's being reverted and moved to gem build -C in the next rubygems release anyways. And it would stil work with this PR even if that change was not being reverted.

To sum up, this PR should be good to go as is!

[deivid-rodriguez]

Thank you @colby-swandale! Fixed conflicts, will merge later 👍.

[deivid-rodriguez]

Going in!

@bundlerbot r+

[ghost]

Build succeeded

• continuous-integration/travis-ci/push