Remove lockfile incompatibility created by the lockfile_uses_separate_rubygems_sources setting#7007
Conversation
The `lockfile_uses_separate_rubygems_sources` was causing a lockfile incompatibility but in my opinion, this incompatibility is not necessary in the general case.
|
I'd love some input from @segiddins here. |
|
This PR is not ready yet, a lot more test code can be removed if we do this. But I would love to hear some opinions about this before moving it forward. |
| [[default_source], @rubygems_sources, git_sources, path_sources, plugin_sources].map do |sources| | ||
| sources.sort_by(&:to_s) | ||
| end.flatten(1) | ||
| lock_sources + rubygems_sources.sort_by(&:to_s) |
There was a problem hiding this comment.
Here I'm reusing the rubygems_sources method in this file, which is defined as
def rubygems_sources
@rubygems_sources + [default_source]
end
Maybe I should undo that change to make this diff more clear.
There was a problem hiding this comment.
So, it's the same 5 components that existed previously ([[default_source], @rubygems_sources, git_sources, path_sources, plugin_sources]), I'm just changing the order to match the previous order.
The difference from before is still maintained: we have separated rubygems sources since the combine_rubygems_sources method that merged them into a single section is no longer used.
|
I agree with the proposed change in general. But i'll need a bit of time to go through the PR and make sure everything looks ok in the next few days. |
|
Thanks for having a look! ❤️ I'm going to get in ready for a final review then, so you can carefully look through the finished version. |
|
I had another look and this is actually ready. I thought more bundler 1/2 specs could be "merged", but the ones that are not merged is because their lockfiles have other incompatibilities still present (the |
|
i would be more comfortable if @segiddins had a look as well before this is merged to just double/triple check everything looks good. |
|
Yeah, I requested his review as soon as I opened this PR, but he must be quite busy... 😞. |
|
Even if @segiddins is not very involved right now, he has been stepping in when he has seen something concerning. So I guess I'm not doing something superbad here... 😅. Maybe we can try ask @indirect have a look? |
|
Isn’t this a breaking change? |
|
I don't think so, I would say it's the opposite. We haven't yet released this new format, and what I'm trying to say here is that changing the format is not necessary. |
|
When I say "new format" I mean "new order of the lockfile sections". |
|
Didn’t we switch to the new format in 2.0? I made this change at André’s request, since it seems a strictly superior ordering of the sources, and the most likely to put the bulk of the gems at the top of the lockfile, rather than in the middle |
No, we didn't.
I did quite a big of git history digging and couldn't find any reason or mention for this new ordering, so I assumed it was not intentional 🤷♂️. It was introduced here: #5792. I personally don't think changing this provides any benefit for our users or for ourselves, so I prefer to keep the same lockfile format. I'm all for breaking changes in the lockfile format if they are necessary, but in this case I see no reason to introduce all this churn in every lockfile that upgrades. |
|
I also wonder why we have separate |
|
The separate settings are because they were built separately, as we gained more understanding of what needed to be done to completely resolve the possible security issue. I think combining the two settings is a reasonable thing to do. I think keeping close to the same ordering to reduce the size and confusion of git diffs for the lock file sounds good, and worth doing. My primary question about this PR is what we mean by "backwards compatible". To fix the security issue, we must separate each source in the lock file. That means Bundler 3 cannot safely install Bundler 2 lockfiles—it has to upgrade them and separate the source sections. Will this change make Bundler 1/2 able to install Bundler 3 lockfiles? If it can, does that mean Bundler 3 will install Bundler 3 lockfiles without a security problem, but Bundler 1/2 will install lockfiles with a security problem? Are we okay with allowing that? |
But they were introduced in the same PR... 🤷♂️
I expect nothing to change regarding the security issue, keeping the different rubygems sources separate is what the PR I linked to fixed, and that should be kept in this PR. The only thing this PR changes is the ordering of sections. So, to give you an example, a lockfile that previously was something like would become Thus keeping the same section type ordering that's already there in the current lockfiles. |
|
What I'm not clear about is whether the fix was actually released, since it's behind a feature toggle... 😬. This PR does not change that anyways. |
Huh, ok. No idea, then! 😬 Maybe so that we could try out the results separately.
I think we probably shipped a version of Bundler that included the feature flag, but we never told people to turn it on? I consider it currently "unshipped", for whatever that is worth.
That makes sense. My question is about what you said in the PR description: "all lockfiles in the world will become incompatible with the previous version". I have some specific questions about that:
|
|
Oh, that was just me being terribly inaccurate, sorry. So, what I meant is that while the original fix was meant for Gemfiles with multiple top level sources, the fix ended up affecting the lockfile format of every lockfile (due to the different ordering being restored here) , and that's not necessary. I didn't mean incompatibility in the sense that bundler will error or not understand the format.
I have no idea... 😬
I kind of brought up this topic in #7084. In this case (similarly to that PR), I wonder if we should maybe remove the feature flags and related settings and check straight for the major version, because once we change this, we don't really want users to opt out. |
Haha okay, that's fair!
Yes, I agree with this. The feature flag exists so that the changes can merge into master, but we want lockfiles from Bundler 3 to require Bundler 3+. We also don't want to let users opt out of the security fix. 😄 |
I just tried it. bundler 1/2 will work just fine with those lockfiles. It will just rewrite them back to the "unified format" bundler 1/2 prefers. |
|
Ok, that’s good to know. We will want to rely on the major version check to enforce Bundler 3+ for Bundler 3 lockfiles, but that seems reasonable to me. |
|
Yup, that makes sense. Thanks for the discussion everyone, let's do this. @bundlerbot r+ |
7007: Remove lockfile incompatibility created by the `lockfile_uses_separate_rubygems_sources` setting r=deivid-rodriguez a=deivid-rodriguez This is more of a question PR, I created this patch to try it out and try to understand, not necessarily get it merged. ### What was the end-user problem that led to this PR? The problem was that once we enable the `lockfile_uses_separate_rubygems_sources` setting, all lockfiles in the world will become incompatible with the previous version. Actually, not necessarily incompatible, but bundler will reorder the sections when the setting is enabled, that will generate churn lock file diffs, and _maybe_ some confusion / merge conflicts, and so on. ### What was your diagnosis of the problem? My diagnosis was that maybe this is not necessary. I read over the issues where this setting was added and what I understood is that previously if a Gemfile specified multiple rubygems sources, they would all get merged together and that's dangerous because it's not deterministic from which source each gem will be picked up, and that could be maliciously exploited. So now each source gets its own separate section. However, how does that affect the ordering of the sections? I don't think it should affect it? ### What is your fix for the problem, implemented in this PR? My fix is to change the `lock_sources` method so that both code branches (`lockfile_uses_separate_rubygems_sources == true`, and `lockfile_uses_separate_rubygems_sources == false`) result in the same ordering of the source sections. ### Why did you choose this fix out of the possible options? I chose this fix because I _think_ it keeps the setting doing the same thing, but also keeps lock file compatibility. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Build succeeded |
ghost
merged commit 
4 participants
This is more of a question PR, I created this patch to try it out and try to understand, not necessarily get it merged.
What was the end-user problem that led to this PR?
The problem was that once we enable the
lockfile_uses_separate_rubygems_sourcessetting, all lockfiles in the world will become incompatible with the previous version. Actually, not necessarily incompatible, but bundler will reorder the sections when the setting is enabled, that will generate churn lock file diffs, and maybe some confusion / merge conflicts, and so on.What was your diagnosis of the problem?
My diagnosis was that maybe this is not necessary. I read over the issues where this setting was added and what I understood is that previously if a Gemfile specified multiple rubygems sources, they would all get merged together and that's dangerous because it's not deterministic from which source each gem will be picked up, and that could be maliciously exploited. So now each source gets its own separate section. However, how does that affect the ordering of the sections? I don't think it should affect it?
What is your fix for the problem, implemented in this PR?
My fix is to change the
lock_sourcesmethod so that both code branches (lockfile_uses_separate_rubygems_sources == true, andlockfile_uses_separate_rubygems_sources == false) result in the same ordering of the source sections.Why did you choose this fix out of the possible options?
I chose this fix because I think it keeps the setting doing the same thing, but also keeps lock file compatibility.