Skip to content
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.

Sign up for GitHub

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

Jump to bottom

builder: fix build cache hash for broken symlink #34271

Merged
merged 2 commits into from
Oct 31, 2017
Merged

builder: fix build cache hash for broken symlink #34271

merged 2 commits into from
Oct 31, 2017

Conversation

tonistiigi
Copy link
Member

fixes #34260

This makes build cache hash for broken symlink in a directory to match the path of resolved path like it appears in the original tarsum version

moby/builder/remotecontext/archive.go

Lines 113 to 115 in 8c72417

// We set sum to path by default for the case where GetFile returns nil.
// The usual case is if relative path is empty.
return path, nil // backwards compat TODO: see if really needed
. There are still plenty of cases where this isn't quite correct, some are described in moby/buildkit#38 . I think we need to rethink what exact data to hash in there. The current is an accidental combination of some tar headers and automatic symlink following because of the ADD/COPY semantics.

@justincormack @dnephin

Signed-off-by: Tonis Tiigi [email protected]

@tonistiigi tonistiigi requested a review from dnephin as a code owner July 26, 2017 22:54
@tonistiigi
Copy link
Member Author

This broke unit tests that checked for a hash of a missing file. It is possible to determine between a broken followed symlink and missing file but there is no utility function for that atm. I didn't find anything that actually relies on the not-found error being produced so I updated the tests. When this logic gets updated we should make sure the error case is put back.

@AkihiroSuda AkihiroSuda added rebuild/powerpc status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Jul 27, 2017
@dnephin dnephin added status/2-code-review and removed rebuild/powerpc status/failing-ci Indicates that the PR in its current state fails the test suite labels Jul 27, 2017
@dnephin
Copy link
Member

dnephin commented Jul 27, 2017

Looks like windows failure is related?

@vieux
Copy link
Contributor

vieux commented Aug 3, 2017

ping @tonistiigi ^

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Aug 6, 2017
@thaJeztah
Copy link
Member

ping @tonistiigi should someone carry this?

/cc @johnstep @salah-khan

@tonistiigi tonistiigi force-pushed the build-symlink-hash branch 3 times, most recently from 8907799 to a77f078 Compare October 2, 2017 18:26
@tonistiigi
Copy link
Member Author

tonistiigi commented Oct 3, 2017 •
edited
Loading

Fixed the unit test. CI still red but this time I don't think it is related.

@tonistiigi tonistiigi removed rebuild/windowsRS1 status/failing-ci Indicates that the PR in its current state fails the test suite labels Oct 3, 2017
@thaJeztah
Copy link
Member

Windows failure is now;

00:12:48 ----------------------------------------------------------------------
00:12:48 FAIL: docker_api_logs_test.go:18: DockerSuite.TestLogsAPIWithStdout
00:12:48 
00:12:48 docker_api_logs_test.go:50:
00:12:48     c.Fatal("timeout waiting for logs to exit")
00:12:48 ... Error: timeout waiting for logs to exit
00:12:48

Which was marked racey; #33301

I'll trigger CI once more for good measures

@thaJeztah thaJeztah mentioned this pull request Oct 3, 2017
Closed
@vieux
Copy link
Contributor

vieux commented Oct 3, 2017

ok looks like the windows failure is racey

@thaJeztah
Copy link
Member

ping @AkihiroSuda could you have a look at this one?

AkihiroSuda
AkihiroSuda reviewed Oct 6, 2017
View reviewed changes
builder/remotecontext/lazycontext.go Outdated
if err != nil {
return "", errors.WithStack(convertPathError(err, cleanPath))
return relPath, nil
Copy link
Member

Choose a reason for hiding this comment

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

if err != nil { return nil } looks as if it were a typo. Please add a comment so that readers won't get confused?

@AkihiroSuda
Copy link
Member

ping @tonistiigi

LGTM after adding the comment

@thaJeztah
Copy link
Member

ping @justincormack PTAL

@yongtang yongtang merged commit 3ab20a8 into moby:master Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@justincormack justincormack justincormack approved these changes

@dnephin dnephin Awaiting requested review from dnephin

Assignees

@crosbymichael crosbymichael

Labels
area/builder status/4-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

symlinks not part of build context hash
9 participants
@tonistiigi @dnephin @vieux @thaJeztah @AkihiroSuda @justincormack @crosbymichael @yongtang @GordonTheTurtle