Skip to content
This repository has been archived by the owner. It is now read-only.

Downgrade version mismatches to warnings #6996

Merged
7 commits merged into from Apr 27, 2019

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Feb 27, 2019

Closes #6993.
Complements rubygems/rubygems#2621.

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

The problem was that bundler 2 has become too unfriendly because it errors out when the running version does not match the version in the BUNDLED WITH section in the lockfile.

What was your diagnosis of the problem?

My diagnosis is that we still believe that making sure the exact same bundler version is always run for a specific application is a good thing, because it ensures resolution is always the same because it's resolved by the exact same code that generated the lockfile.

BUT, we need to ensure this in a more user friendly way, for example by auto-switching and auto-installing the right bundler version without any user intervention.

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

My fix is to downgrade this hard error to a warning that does not prevent bundler from running.

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

I chose this fix because it still shows the mismatch but allows bundler to run, thus being a bit friendlier. Hopefully this won't be needed once bundler is smart enough to autoswitch and autoinstall.

@deivid-rodriguez deivid-rodriguez force-pushed the downgrade_version_mismatches_to_warnings branch 2 times, most recently from a461aa7 to 4f8ad84 Compare Feb 27, 2019
@deivid-rodriguez deivid-rodriguez requested a review from indirect Feb 27, 2019
@p-mongo
Copy link

p-mongo commented Feb 27, 2019

+1 for downgrade, automatic installation of software from network? Sounds like something I would like to NOT have on my machines (I am not a windows/apple user, thanks).

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Feb 27, 2019

I don't yet have a clear idea of how it will exactly work, but my initial intention was that this "self auto-installation" would happen on bundle install. So I think it wouldn't be surprising for users because we are installing software from the network in any case.

@colby-swandale colby-swandale added this to the 2.0.2 milestone Mar 12, 2019
@colby-swandale
Copy link
Member

colby-swandale commented Mar 12, 2019

I'm kinda torn on this. I'm worried about how users could be using a version of Bundler that includes breaking changes without realising it. I get the argument that lots of users were running into this error, but it made users aware that they were trying to use a Gemfile against a new version of Bundler that had breaking changes.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 12, 2019

But don't you think aborting is too much? Specially for the bundler 1 -> bundler 2 transition, since those are not intended to be really backwards incompatible (except for dropping old rubies, which does not affect supported rubies).

As a matter of fact, the only thing that makes bundler 2 incompatible with bundler 1 is this issue... 😅.

@indirect
Copy link
Member

indirect commented Mar 12, 2019

I'm also torn. I agree with @deivid-rodriguez that it is not user-friendly to throw an error when the installation might succeed, and I also agree with @colby-swandale that it's worrying to let users do things that we know sometimes won't work at all.

If we are going to allow users to do something that we know will sometimes fail and/or throw mysterious errors, I think that probably deserves a different kind of warning than "you have 1.17.2, and this lockfile was created with 1.17.3".

@deivid-rodriguez @colby-swandale how do you feel about these two options?

  1. We could add something to the friendly exception catcher that look for a major version mismatch. That way if there is an exception, the end user will see something like "MAJOR VERSION MISMATCH: you are using Bundler 2 on a Bundler 1 lockfile. You might be able to resolve this error by running `gem install bundler -v '~> 1' and trying again."
  2. We could change the version mismatch warning when it is a major version. Instead of saying "We suggest you upgrade", we could say something like "MAJOR VERSION MISMATCH: you are using Bundler 2 on a Bundler 1 lockfile, which is not supported by the Bundler team. This might not work. If you have problems, install Bundler 1 by running gem install bundler -v '~> 1' and try again.

What do you think? I'm trying to figure out how to balance the way user-friendliness means getting out of the way and "just working" with the way that Bundler 2 doesn't always actually work the same way as Bundler 1. 😬 Also super open to ideas. 😄

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 12, 2019

Wait, just to clarify.

In this PR, I'm not actually changing how this works, I'm only delaying it to the next major version bump. This is because the bundler 2 major release was special in the sense that there were not strict intended backwards incompatible changes. We decided to release it in order to get rid of old rubies support, but other than that there should've been no breaking changes (other than this issue being overlooked).

So, we do kwow that installation will work, at least with the same certainty that a minor or patch level version bump will keep things working. So, I downgraded the hard error to the warning we already print when there's a mismatch in the minor or the patch level version.

I do think that the "hard error" message can be improved with actionable suggestions (the rubygems part of this already has that), but for now my idea was just to delay the hard errors.

@indirect
Copy link
Member

indirect commented Mar 12, 2019

Yep, I definitely understand that this is delaying the hard error. To me, this issue feels like tension between being user friendly (not throwing a hard error because it MIGHT work) and being Bundler-dev-team friendly (not letting users do this, because it might NOT work).

Bundler 2 should hopefully be almost entirely compatible, but we aren't supporting it that way. I'm worried that if we merge this and ship it, we'll start getting new issues where Bundler 2 doesn't work the same as Bundler 1. The options I suggested above are me trying to solve that worry, while still reducing the v2 vs v1 error to a warning.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 12, 2019

Sure, I can understand that.

Should I close this PR and it's rubygems counterpart, and focus on more actionable error messages. Just note that the case this was solving is not the one you mention in your suggestions, but the inverse (running bundler 1 on a bundler 2 lockfile). But for this fix to make sense in that case, we would need to backport this to Bundler 1... 🤔.

@indirect
Copy link
Member

indirect commented Mar 18, 2019

Hmm. Maybe we should explicitly write out our warning/error goals, and then work on PRs to get there? I don't think we need to get rid of this PR, but I want to make sure it will have the end results that we actually want before we merge it. There are four cases (I think): Bundler 1 or Bundler 2, with a v1 lock or v2 lock. Here is my proposal for how we ideally want each case to work:

  • Bundler 1, v1 lock: just works, nothing special
  • Bundler 1, v2 lock: prints a yellow warning MAJOR VERSION MISMATCH: This is a Bundler 2 lockfile, but you have Bundler 1. This may not work, and the Bundler team cannot support you if you have problems. Upgrade to Bundler 2 by running "gem install bundler" to resolve this warning.
  • Bundler 2, v1 lock: prints a yellow warning MAJOR UPGRADE: This is a Bundler 1 lockfile, but you are running Bundler 2. This may not work, and the Bundler team cannot support you if you have problems. Upgrade your lockfile by running "bundle update --bundler" to resolve this warning.
  • Bundler 2, v2 lock: just works, nothing special

How do you all feel about that, @deivid-rodriguez @colby-swandale @hsbt?

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 19, 2019

Thanks @indirect. Sounds good, I will have a look at the different cases and what kind of message we are giving in each case.

But just to make sure we are all on the same page, because I feel we are going a bit back and forth here. Do we agree all agree with the intention of this PR, namely, downgrading hard errors about this to warnings, only for the bundler 2 release?

@indirect
Copy link
Member

indirect commented Mar 19, 2019

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 19, 2019

Yup, that's the intention, no lock file upgrade, just a warning printed and lock file left alone 👍. I will double check when I check all the different combinations you mentioned before.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 23, 2019

Sorry for the delay here. rubygems/rubygems#2621 has been merged so this is next on my plate 👍.

@colby-swandale
Copy link
Member

colby-swandale commented Mar 23, 2019

@indirect In the current implementation the Bundler 1 Gemfile.lock will automatically be updated to Bundler 2 without the user's consent.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 23, 2019

You mean in which implementation? This PR, latest release, master, 2-0-stable?

@colby-swandale
Copy link
Member

colby-swandale commented Mar 24, 2019

Everything. Bundler will just lock to whatever version is running bundle install. So going from 1.17.2 -> 2.0.1 with bundle install will happen.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 24, 2019

Really? I don't think I've seen that behavior. I have several apps/libraries locked to bundler 1, and they don't get upgraded silently unless I use bundle update --bundler. Have you double checked this?

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 24, 2019

$ docker run --rm -it ruby:2.6.2 sh

# bundle --version
Bundler version 1.17.2

# gem --version
3.0.3

# touch Gemfile              

# bundle
The Gemfile specifies no dependencies
Resolving dependencies...
Bundle complete! 0 Gemfile dependencies, 1 gem now installed.
Bundled gems are installed into `./usr/local/bundle`

# cat Gemfile.lock	
GEM
  specs:

PLATFORMS
  ruby

DEPENDENCIES

BUNDLED WITH
   1.17.2

# gem install bundler:2.0.1
Fetching bundler-2.0.1.gem
Successfully installed bundler-2.0.1
1 gem installed

# bundle --version
Bundler version 1.17.2

# bundle install
The Gemfile specifies no dependencies
Bundle complete! 0 Gemfile dependencies, 1 gem now installed.
Bundled gems are installed into `./usr/local/bundle`

# cat Gemfile.lock
GEM
  specs:

PLATFORMS
  ruby

DEPENDENCIES

BUNDLED WITH
   1.17.2

@colby-swandale
Copy link
Member

colby-swandale commented Mar 25, 2019

$ cat Gemfile.lock
GEM
  remote: https://rubygems.org/
  specs:
    rack (2.0.6)

PLATFORMS
  ruby

DEPENDENCIES
  rack

BUNDLED WITH
   1.17.2

$ bundle --version
Bundler version 1.17.2
$ git checkout master
$ ruby -I lib exe/bundle install
Fetching gem metadata from https://rubygems.org/..............
Resolving dependencies...
Fetching rack 2.0.6
Installing rack 2.0.6
Warning: the lockfile is being updated to Bundler 2, after which you will be unable to return to Bundler 1.
Bundle complete! 1 Gemfile dependency, 2 gems now installed.
Bundled gems are installed into `./.bundle`
$ cat Gemfile.lock
GEM
  remote: https://rubygems.org/
  specs:
    rack (2.0.6)

PLATFORMS
  ruby
  x86_64-darwin-18

DEPENDENCIES
  rack

BUNDLED WITH
   2.0.0.dev

@colby-swandale
Copy link
Member

colby-swandale commented Mar 25, 2019

Note that this PR doesn't change any of this behavior already.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 25, 2019

Ok, so the answer to my previous question would be that this is the behaviour in master, but not in any released version?

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 25, 2019

Yeah, I reproduced this on master. This is unexpected to me. Do we all agree that we don't want this?

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 25, 2019

Ok, I see what's going on now.

The selection of the right bundler executable for an application is currently done by rubygems. If we run the bundler executable directly without going through a rubygems binstub, we skip that step, that's why running ruby -Ilib exe/bundle upgrades the lockfile. But if you do gem build bundler.gemspec; gem install bundler-2.0.0-dev.gem, then things will happen as expected.

@colby-swandale colby-swandale removed this from the 2.0.2 milestone Apr 6, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Apr 8, 2019

What's the plan here? Should I rebase this PR and review the diffferent situations @indirect mentioned so that we can ship this with the next patch level release?

@eregon
Copy link
Contributor

eregon commented Apr 14, 2019

Running a bundler 2 gemfile with bundler 3 prints this warning: "Warning: the lockfile is being updated to Bundler 3, after which you will be unable to return to Bundler 2.", but works.

I think it should not update the lockfile, otherwise it makes it incompatible with Bundler 2, and it forces everyone using the lockfile to update to Bundler 3.
Bundler 3 should be free to change the lockfile format if it needs to, and Bundler 2 would not be able to understand that.

@eregon
Copy link
Contributor

eregon commented Apr 14, 2019

"Warning: the lockfile is being updated to Bundler 3, after which you will be unable to return to Bundler 2."

This message also seems to imply Running bundler 2 / Locked to Bundler 3 will result in an error, which I think is OK in this direction.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Apr 14, 2019

In general, the lock file will be updated with the bundler version that created it, that's why it's called the BUNDLED WITH section. I think the only exception to this is when an older version is running a newer lockfile. In that case, bundler is allowed to run but the locked version is not changed and the user gets a warning message. This is already the case for minor/patch level version mismatches.

This PR intends to extend that ONLY for bundler 1/2 major version mismatches, as a way to try to alleviate the issues we've been having while the lockfile format having no intended incompatibilities. But keeps the error for bundler 3 or higher.

(By the way, I think the implementation of the PR is incorrect 😅, thanks for making me revisiting it). I think it's correct after all, too many moving parts!

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Apr 14, 2019

I corrected the table once again to reflect this 🙈.

@deivid-rodriguez deivid-rodriguez force-pushed the downgrade_version_mismatches_to_warnings branch from aedcee2 to 03ac322 Compare Apr 14, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Apr 14, 2019

We might want to evaluate the possibility of forcing users with a Bundler 3 lock to upgrade to Bundler 3, because installing a Bundler 3 lock with Bundler <=2 could expose users to new security issues that they thought they were safe from by having a Bundler 3 lock.

@indirect The table was incorrect, this PR only adds a special case for bundler 1 and 2, but this is still the same otherwise. Users running a bundler 3+ lockfile with bundler 1 or 2 will get an error telling them to install bundler 3.

@eregon
Copy link
Contributor

eregon commented Apr 14, 2019

The table makes sense to me now 😄

In general, the lock file will be updated with the bundler version that created it, that's why it's called the BUNDLED WITH section.

Right, I understand BUNDLED WITH as the version of bundler that did the resolution and created the lockfile.
However, running bundle _2.0.1_ install with a lockfile of Bundler 1 does not create the lockfile and does not resolve, so I think it should not change the lockfile BUNDLED WITH.

I think the only exception to this is when an older version is running a newer lockfile (same major version).

Right, that warns and does not change the lockfile, as I expect.
Another case is same major, but bundler newer than the lockfile. This is of course fine, and the lockfile is not changed.
I think Bundler 2 and lockfile 1 is a very similar situation. IMHO bundle install could warn that you might want to redo resolution with Bundler 2, but should not change the lockfile.

This PR intends to extend that ONLY for bundler 1/2 major version mismatches, as a way to try to alleviate the issues we've been having while the lockfile format having no intended incompatibilities. But keeps the error for bundler 3 or higher.

I see, makes sense.

"bundler:#{bundler_version}#{prerelease_text}`.\n"
end
current_major_version = current_version.segments.first
major_bundler_version = bundler_version.segments.first
Copy link
Contributor

@eregon eregon Apr 14, 2019

Choose a reason for hiding this comment

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

bundler_version being the BUNDLED WITH version is a very confusing name (it's not Bundler's version!).
The code was already like that, but it definitely doesn't make it easy to understand what happens.

Copy link
Member Author

@deivid-rodriguez deivid-rodriguez Apr 14, 2019

Choose a reason for hiding this comment

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

Yes, absolutely agree. I left it there because it was already like that, but should be changed. I can do that.

end
current_major_version = current_version.segments.first
major_bundler_version = bundler_version.segments.first
if current_major_version < major_bundler_version && current_major_version > 2
Copy link
Contributor

@eregon eregon Apr 14, 2019

Choose a reason for hiding this comment

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

Should be major_bundler_version > 2, right?

Copy link
Member Author

@deivid-rodriguez deivid-rodriguez Apr 14, 2019

Choose a reason for hiding this comment

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

I had the exact same thought, but maybe it's because of the naming confusion? I had a second look before and decided it was correct.

Copy link
Contributor

@eregon eregon Apr 14, 2019

Choose a reason for hiding this comment

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

current_major_version is the Bundler (major) version, right?
And major_bundler_version is the (major) BUNDLED WITH version.
We want to error out if Bundler version < BUNDLED WITH and BUNDLED WITH > 2 (all BUNDLED WITH <= 2 lockfiles are compatible).

I believe if Bundler version < BUNDLED WITH and Bundler version > 2 is incorrect, for instance for Bundler version 2 (or 1) and BUNDLED WITH 3, which should error out but this condition is false.

Copy link
Member Author

@deivid-rodriguez deivid-rodriguez Apr 14, 2019

Choose a reason for hiding this comment

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

Right, this is so confusing... The thing (I believe) is that the lower left part of the table should be already caught by the rubygems. The error would be:

Could not find 'bundler' (3) required by your Gemfile.lock. To update to the latest version installed on your system, run bundle update --bundler. To install the missing version, run gem install bundler:3

Downgrading that to a warning as well is the complementary part of this in rubygems: rubygems/rubygems#2696.

This brings up other issues:

  • We should probably remove the bundle update --bundler recommendation from that advice, since I don't think bundle update --bundler works for downgrading the BUNDLED WITH version.

  • We might want to remove the check in those cases, because otherwise, with both PRs applied, you get duplicated warnings, like

$ bundle 
Could not find 'bundler' (3.0.0) required by your /home/deivid/Code/playground/bundler/bundle_with_dance/Gemfile.lock.
To update to the latest version installed on your system, run `bundle update --bundler`.
To install the missing version, run `gem install bundler:3.0.0`

Warning: the running version of Bundler (2.1.0.pre.1) is older than the version that created the lockfile (3.0.0).
We suggest you to upgrade to the version that created the lockfile by running `gem install bundler:3.0.0`.
The Gemfile specifies no dependencies
Bundle complete! 0 Gemfile dependencies, 1 gem now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.

Copy link
Member Author

@deivid-rodriguez deivid-rodriguez Apr 14, 2019

Choose a reason for hiding this comment

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

Maybe we should actually remove this check altogether from bundler and unify everything in rubygems.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Apr 14, 2019

Right, I understand BUNDLED WITH as the version of bundler that did the resolution and created the lockfile.
However, running bundle 2.0.1 install with a lockfile of Bundler 1 does not create the lockfile and does not resolve, so I think it should not change the lockfile BUNDLED WITH.

Yeah, that's what I meant too. Version should be changed only if the bundle command ends up rewriting the lockfile. That part is currently confusing in the table, so I'll just remove it.

@indirect
Copy link
Member

indirect commented Apr 22, 2019

After further discussions, I am going to write up a proposal for Bundler to always accept existing lockfiles, without errors. It seems like the best way to achieve our compatibility goals is to only “upgrade” the lockfile when asked to do so by the user.

@deivid-rodriguez deivid-rodriguez force-pushed the downgrade_version_mismatches_to_warnings branch 2 times, most recently from aba7f18 to be71b55 Compare Apr 26, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Apr 26, 2019

@indirect Whatever the proposal ends up being, I noticed that this hard error shouldn't be here. Whether you require bundler/setup, or run bundler standalone, both cases should be caught by the bundler version finder in rubygems, so I don't think we need the hard error here. So, I removed it.

@indirect
Copy link
Member

indirect commented Apr 26, 2019

Works for me. :) I'm r+ on this and we can do a follow-up once the RFC is reviewed and accepted.

This is essentially the same as "errors if the current is a major
version older than lockfile's bundler version".
This is already short-circuit by rubygems' bundler version finder.
The simulation is not very friendly because it needs to happen before
`hax.rb` is loaded so, for example, it won't be applied to the lockfile
platforms.
@deivid-rodriguez deivid-rodriguez force-pushed the downgrade_version_mismatches_to_warnings branch from be71b55 to 7ca4364 Compare Apr 27, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Apr 27, 2019

Ok! Let's do this then!

@bundlerbot r=indirect

ghost pushed a commit that referenced this issue Apr 27, 2019
6996: Downgrade version mismatches to warnings r=indirect a=deivid-rodriguez

Closes #6993.
Complements rubygems/rubygems#2621.

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

The problem was that bundler 2 has become too unfriendly because it errors out when the running version does not match the version in the BUNDLED WITH section in the lockfile.

### What was your diagnosis of the problem?

My diagnosis is that we still believe that making sure the exact same bundler version is always run for a specific application is a good thing, because it ensures resolution is always the same because it's resolved by the exact same code that generated the lockfile.

BUT, we need to ensure this in a more user friendly way, for example by auto-switching and auto-installing the right bundler version without any user intervention.

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

My fix is to downgrade this hard error to a warning that does not prevent bundler from running.



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

I chose this fix because it still shows the mismatch but allows bundler to run, thus being a bit friendlier. Hopefully this won't be needed once bundler is smart enough to autoswitch and autoinstall.


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
@ghost
Copy link

ghost commented Apr 27, 2019

Build succeeded

@ghost ghost merged commit 7ca4364 into master Apr 27, 2019
3 checks passed
@ghost ghost deleted the downgrade_version_mismatches_to_warnings branch Apr 27, 2019
This pull request was closed.
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.

5 participants