Skip to content
This repository was archived by the owner on Apr 14, 2021. It is now read-only.

Avoid hitting RubyGems remotes when all missing gems are in --without groups#5806

Merged
bundlerbot merged 4 commits intomasterfrom
seg-dont-hit-remote-twice
Jun 23, 2017
Merged

Avoid hitting RubyGems remotes when all missing gems are in --without groups#5806
bundlerbot merged 4 commits intomasterfrom
seg-dont-hit-remote-twice

Conversation

@segiddins
Copy link
Copy Markdown
Contributor

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

The problem was we would attempt to fetch dependency info from a RubyGems remote even when it was only needed for gems whose groups were excluded.

Was was your diagnosis of the problem?

My diagnosis was that the test we had for this case was insufficient, since it checked only that stderr was empty, and since Bundler 1 prints errors to stdout... it went undetected for a year.

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

My fix is to only report that deps are missing when they are either requested or are path/git/gemspec-sourced.

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

I chose this fix because it keeps the behavior of always ensuring all git repos are checked out.

@indirect
Copy link
Copy Markdown

Any chance we can also skip checking out git sources if they only contain gems that are excluded? That would fix the regression I found in 1.8 last year :P

@segiddins
Copy link
Copy Markdown
Contributor Author

Any chance we can also skip checking out git sources if they only contain gems that are excluded? That would fix the regression I found in 1.8 last year :P

I don't know how well I can make that work with rspec ./spec/install/git_spec.rb:39 # bundle install git sources should check out git repos that are missing but not being installed

@indirect
Copy link
Copy Markdown

@segiddins that's when there's no lockfile and Bundler needs to use the gemspec from that git repo to successfully resolve. I'm talking about when we're running in deployment/frozen mode, and there's already a lock, so we don't actually need the gemspec for the without gem.

@segiddins segiddins force-pushed the seg-dont-hit-remote-twice branch from 81077bd to 65aecf5 Compare June 22, 2017 00:51
@segiddins
Copy link
Copy Markdown
Contributor Author

@indirect I think I did it?

@indirect
Copy link
Copy Markdown

🎉

@segiddins
Copy link
Copy Markdown
Contributor Author

r?

@indirect
Copy link
Copy Markdown

My test for the regression is still failing inside Definition#converge_paths, where specs_changed? requires the directory to exist. Are we checking every source for changes, whether or not we need any gems from that source, even in frozen mode?

@indirect
Copy link
Copy Markdown

(The repro is here if you want to see it for yourself: https://gist.github.com/indirect/1cc86d686f40a40d68b895834ab11cf6)

@segiddins
Copy link
Copy Markdown
Contributor Author

That looks like a different issue? That gemfile doesn't have any Git sources.

@segiddins
Copy link
Copy Markdown
Contributor Author

Lol I think I have a fix for it anyways

@indirect
Copy link
Copy Markdown

lol 🙌🏻

@segiddins
Copy link
Copy Markdown
Contributor Author

@indirect take a look :P

@segiddins segiddins force-pushed the seg-dont-hit-remote-twice branch from 65aecf5 to 1947060 Compare June 22, 2017 16:49
@indirect
Copy link
Copy Markdown

you did it! 🎉🎉🎉

@indirect
Copy link
Copy Markdown

@bundlerbot r+

@bundlerbot
Copy link
Copy Markdown
Collaborator

📌 Commit 1947060 has been approved by indirect

bundlerbot added a commit that referenced this pull request Jun 23, 2017
Avoid hitting RubyGems remotes when all missing gems are in --without groups

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

The problem was we would attempt to fetch dependency info from a RubyGems remote even when it was only needed for gems whose groups were excluded.

### Was was your diagnosis of the problem?

My diagnosis was that the test we had for this case was insufficient, since it checked only that `stderr` was empty, and since Bundler 1 prints errors to `stdout`... it went undetected for a year.

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

My fix is to only report that deps are missing when they are either requested or are path/git/gemspec-sourced.

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

I chose this fix because it keeps the behavior of always ensuring all git repos are checked out.
@bundlerbot
Copy link
Copy Markdown
Collaborator

⌛ Testing commit 1947060 with merge 1102ec1...

@bundlerbot
Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-travis
Approved by: indirect
Pushing 1102ec1 to master...

@bundlerbot bundlerbot merged commit 1947060 into master Jun 23, 2017
@segiddins segiddins deleted the seg-dont-hit-remote-twice branch June 23, 2017 15:01
@segiddins segiddins added this to the 1.15.2 milestone Jul 5, 2017
@segiddins segiddins removed this from the 1.15.2 milestone Jul 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants