Skip to content

builder: fix build cache hash for broken symlink#34271

Merged
yongtang merged 2 commits into
moby:masterfrom
tonistiigi:build-symlink-hash
Oct 31, 2017
Merged

builder: fix build cache hash for broken symlink#34271
yongtang merged 2 commits into
moby:masterfrom
tonistiigi:build-symlink-hash

Conversation

@tonistiigi
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi commented Jul 26, 2017

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
Copy Markdown
Member Author

tonistiigi commented Jul 26, 2017

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
Copy Markdown
Member

dnephin commented Jul 27, 2017

Looks like windows failure is related?

@vieux
Copy link
Copy Markdown
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 thaJeztah added the area/builder Build label Sep 29, 2017
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Sep 29, 2017

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
Copy Markdown
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
Copy Markdown
Member

thaJeztah commented Oct 3, 2017

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
Copy Markdown
Contributor

vieux commented Oct 3, 2017

ok looks like the windows failure is racey

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 5, 2017

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

AkihiroSuda
AkihiroSuda reviewed Oct 6, 2017
View reviewed changes
Comment thread builder/remotecontext/lazycontext.go Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Oct 6, 2017

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
Copy Markdown
Member

AkihiroSuda commented Oct 10, 2017

ping @tonistiigi

LGTM after adding the comment

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 31, 2017

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

@dnephin dnephin Awaiting requested review from dnephin

+1 more reviewer

@justincormack justincormack justincormack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@crosbymichael crosbymichael

Labels

area/builder Build 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