-
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
git: support subdir component #2116
Conversation
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
This will be really useful for what I've been working on, thanks! Do we need to back port this? Could it not just land in the next minor release? |
return nil, errors.Wrapf(err, "failed to create temporary checkout dir") | ||
} | ||
} | ||
_, err = gitWithinDir(ctx, gitDir, cd, sock, knownHosts, nil, "checkout", ref, "--", ".") |
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.
Wonder is a sparse-checkout wouldn't be more efficient in this case but would require at least Git 2.25.0 which is not widely used atm unfortunately. Maybe this could be fixed with #1048 when go-git/go-git#90 is implemented.
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.
This can be looked at later as an optimization but may be tricky because of the shared local repository buildkit uses for incremental updates(or this would maybe need to be turned off for subdirs then). Also, not quite sure how cleanly fallback can be done for servers that don't support it.
} | ||
} | ||
if err := d.Close(); err != nil { | ||
return nil, err |
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.
Wouldn't d.Close()
be called again here?
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.
The close in defer is skipped in here
I think we can go on with this as is. Adding llb fallback for this is tricky because caps are not known until marshaling and completely different llb is needed for fallback. I did make a commit with fallback in the frontend but it is kind of pointless because to parse |
That sounds good to me @tonistiigi! |
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.
The path separator stuff probably does not work on Windows, so perhaps we should have "!windows" build tag?
Why would it not work on windows? |
fixes #1684
wip: trying to see if I can also add backward compatibility to support old buildkit versions in https://github.com/moby/buildkit/compare/tonistiigi:git-subdir-wip. Looks tricky but may be possible.
@crazy-max @chris-crone
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com