-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: dot and empty paths normalized correctly for COPY to WORKDIR #5080
Conversation
5cd38aa
to
ce59dbf
Compare
While we're here, Lines 77 to 82 in 997156a
|
ce59dbf
to
15624ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need regression integration test for this as well. Is this something that should be handled by pathRelativeToWorkingDir
(that part could be follow up).
45d8652
to
4abc6ad
Compare
@@ -1763,9 +1763,6 @@ func pathRelativeToWorkingDir(s llb.State, p string, platform ocispecs.Platform) | |||
return "", err | |||
} | |||
|
|||
if len(p) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: removed so that empty directory ""
are handled properly, especially for non-root WORKDIR
. That's where the regression happens.
Sure, it's better at Added the integration tests. It was very interesting that the tests were passing even without the fix, if you have |
In my previous fix on moby#4825, I had removed this line knowing that all that had been addressed in `pahtRelativeToWorkingDir`: ```go if cfg.params.DestPath == "." // <-- this one || cfg.params.DestPath == "" || cfg.params.DestPath[len(cfg.params.DestPath)-1] == filepath.Separator { dest += string(filepath.Separator) } ``` However, I had overlooked the `"."` and `""` scenarios. `""`. The `"/"`case is handled correctly in `system.NormalizePath()`. This change therefore undoes this, to make sure "." is transformed correctly to "./" during COPY operation, same to "" -> "./". This is important especially for WORKDIR that are not `/`, so that `COPY --link` operations are handled properly. fixes moby#5070 Signed-off-by: Anthony Nandaa <[email protected]>
4abc6ad
to
c605289
Compare
@tonistiigi - PTAL again. |
In my previous fix on #4825, I had removed this line knowing that all that had been addressed in
pahtRelativeToWorkingDir
:However, I had overlooked the
"."
and""
scenarios.""
. The"/"
case is handled correctly insystem.NormalizePath()
.This change therefore undoes this, to make sure "." is transformed correctly to "./" during COPY operation, same to "" -> "./". This is important especially for WORKDIR that are not
/
, so thatCOPY --link
operations are handled properly.fixes #5070