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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove git subshelling from gemspec #6985

Merged
merged 2 commits into from Mar 9, 2019

Conversation

Projects
None yet
5 participants
@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Feb 22, 2019

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

simi approved these changes Feb 22, 2019

Copy link
Contributor

simi left a comment

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

馃憤

@deivid-rodriguez deivid-rodriguez force-pushed the no_git_on_gemspec branch from 537a500 to f05a45e Feb 22, 2019

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

deivid-rodriguez commented Feb 22, 2019

Yeah, the 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

This comment has been minimized.

Copy link
Member

hsbt commented Feb 22, 2019

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.

@hsbt

hsbt approved these changes Feb 22, 2019

@colby-swandale

This comment has been minimized.

Copy link
Member

colby-swandale commented Feb 22, 2019

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

This comment has been minimized.

Copy link
Member

indirect commented Feb 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

deivid-rodriguez commented Feb 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

deivid-rodriguez commented Feb 23, 2019

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

This comment has been minimized.

Copy link
Member

indirect commented Feb 24, 2019

Sorry my explanation wasn鈥檛 clear! I also meant as a dependency of the rake task named :build 馃檪

@deivid-rodriguez deivid-rodriguez force-pushed the no_git_on_gemspec branch from f05a45e to 3470aef Feb 27, 2019

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

deivid-rodriguez commented Feb 27, 2019

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 deivid-rodriguez force-pushed the no_git_on_gemspec branch from 3470aef to 37bffe1 Mar 3, 2019

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

deivid-rodriguez commented Mar 3, 2019

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 rubygems/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 deivid-rodriguez requested a review from colby-swandale Mar 3, 2019

@deivid-rodriguez deivid-rodriguez force-pushed the no_git_on_gemspec branch from 37bffe1 to bf7bbc8 Mar 5, 2019

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

deivid-rodriguez commented Mar 5, 2019

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

deivid-rodriguez added some commits Feb 22, 2019

@deivid-rodriguez deivid-rodriguez force-pushed the no_git_on_gemspec branch from bf7bbc8 to 0bf5104 Mar 5, 2019

@deivid-rodriguez

This comment has been minimized.

Copy link
Contributor Author

deivid-rodriguez commented Mar 9, 2019

Going in!

@bundlerbot r+

bundlerbot bot pushed a commit that referenced this pull request Mar 9, 2019

Merge #6985
6985: Remove git subshelling from gemspec r=deivid-rodriguez a=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.

Co-authored-by: David Rodr铆guez <deivid.rodriguez@riseup.net>
@bundlerbot

This comment has been minimized.

Copy link
Contributor

bundlerbot bot commented Mar 9, 2019

@bundlerbot bundlerbot bot merged commit 0bf5104 into master Mar 9, 2019

2 checks passed

bors Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bundlerbot bundlerbot bot deleted the no_git_on_gemspec branch Mar 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.