Make bundle lock always touch the lockfile in non-frozen mode
#7220
Conversation
bundler/lib/bundler/definition.rb
Outdated
| @@ -339,7 +339,10 @@ def lock(file, preserve_unknown_sections = false) | |||
|
|
|||
| preserve_unknown_sections ||= !updating_major && (Bundler.frozen_bundle? || !(unlocking? || @unlocking_bundler)) | |||
|
|
|||
| return if file && File.exist?(file) && lockfiles_equal?(@lockfile_contents, contents, preserve_unknown_sections) | |||
| if file && File.exist?(file) && lockfiles_equal?(@lockfile_contents, contents, preserve_unknown_sections) | |||
| FileUtils.touch(file) | |||
There was a problem hiding this comment.
Should I wrap this in a SharedHelpers.filesystem_access block as well? 馃
There was a problem hiding this comment.
Yes, that helper unifies error rescuing for filesystem access problems. Touch is a write to the filesystem.
There was a problem hiding this comment.
Tell me what you think. Should we copy the frozen bundle check to guard the touch? I know for lock it temporarily unfreezes because it's nonsensical otherwise, but this code path will run during install and update, I believe. We should not otherwise change the lockfile if frozen, right?
There was a problem hiding this comment.
You're referring to the little "Cannot write a changed lockfile while frozen" guard directly below, right? Yes, I also caught that and it seemed sensible not to touch the lockfile when frozen, but I did not add an explicit test about that.
There was a problem hiding this comment.
Alright, I applied the wrapper. Anything else?
|
This is great! Thanks for working on this and thanks for including the test coverage. I added some questions inline and then this should be fairly simple to get merged. |
| SharedHelpers.filesystem_access(file) {|p| FileUtils.touch(p) } | ||
| return |
There was a problem hiding this comment.
I would specifically return without touching the lockfile if the bundle is frozen, like so:
| SharedHelpers.filesystem_access(file) {|p| FileUtils.touch(p) } | |
| return | |
| return if Bundler.frozen_bundle? | |
| SharedHelpers.filesystem_access(file) {|p| FileUtils.touch(p) } | |
| return |
There was a problem hiding this comment.
I'd likewise add a test that it is not touched when frozen. Maybe here, since the lock command always temporarily unfreezes.
martinemde
force-pushed
the
fl/7120-bundle-lock-touch
branch
from
c8a9f1b
to
119d4bd
Compare
|
Hey @franzliedke, I decided to go ahead and merge this now since I want to see it make it into the 2.5.0 bundler release. I'll update with the test I was imagining. Thanks for your contribution! |
|
@martinemde Thanks! To be honest, I was hoping this would happen. 馃槄 I wasn't 100% sure about the implications in these other commands that I wasn't aware of. |
deivid-rodriguez
changed the title
bundle lock always touch the lockfile in non-frozen mode
Fixes #7120.
What was the end-user or developer problem that led to this PR?
In #7120, @hadmut mentioned problems with Make-based build pipelines:
Gemfile.lockwould not be "touched" (receive an updated file modification date) when runningbundle lockis a no-op (i.e. the lockfile already matches theGemfile).What is your fix for the problem, implemented in this PR?
An explicit
FileUtils.touchcall. 馃Make sure the following tasks are checked