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.

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

[proposal] delegate build execution to buildx bake #9477

Closed
ndeloof opened this issue May 18, 2022 · 4 comments
Closed

[proposal] delegate build execution to buildx bake #9477

ndeloof opened this issue May 18, 2022 · 4 comments

Comments

@ndeloof
Copy link
Contributor

ndeloof commented May 18, 2022

Description
buildx is used as a vendored dependency to implement compose build command. With lack of a minimal "build" API/SDK, we have to copy/paste some significant amount of code, and to adjust this code to follow updates on buildx development. So we basically are always late to adopt new features.

On the other hand, buildx evolves quickly and introduce many new features which are initially available through the bake command using dedicated HCL syntax. While compose.yaml format is also supported, I don't expect bake to always be up-to-date with the compose specification, and especially being able to introduce new build syntax in the compose-specification would require cross repositories PR which makes our maintenance effort heavier.

My proposal is to re-implement compose build by delegating execution to buildx bake command, generating a bake.hcl configuration file with everything needed based on the compose model, build flags, and specific build conditions (images to (re)build, etc).

This will enforce we get the best of buildx always available to implement compose build, and also defines a clean separation of responsibilities so the buildx maintainers can focus on buildx core features, making builds faster and/or more flexible, while we iterate on the compose syntax and UX.

Additional environment details:
This proposal comes after discussions on #9466 (comment)

@ndeloof
Copy link
Contributor Author

ndeloof commented May 18, 2022

@tonistiigi
Copy link
Member

I don't expect bake to always be up-to-date with the compose specification,

I'm not sure I get this. The main problem is that there shouldn't be duplicate implementation when one side adds a feature and the other needs to port, leading to maintenance overhead and fragmentation for the users. If we know that the implementation is in buildx then this is where the features go. If compose spec adds a new build field then it is either implemented in buildx or it is not implemented at all(as far as this repo is concerned).

My proposal is to re-implement compose build by delegating execution to buildx bake command, generating a bake.hcl configuration file with everything needed based on the compose model, build flags, and specific build conditions

I don't understand why this conversion code would be needed (and if you convert then you generate json, hcl is for humans). If there is a compose feature that doesn't work with bake directly(still not sure) then it is a bad experience for the user. We don't want the case where compose works with bake because of this conversion but bake itself would show some error.

This doesn't mean that I would expect there to be a lot of compose-specific logic in buildx. We are already reusing the spec, parser etc from upstream and that would of course continue to be like this. For the build invocation itself, I think the updates so far have been only added fields and I'd expect it to be similar for the future as well.

From the compose UX side I think this repo should still have full control of that. Eg. I mentioned in #9466 (comment) the possibility to get raw progress from buildx if needed. Compose build does not need to equal buildx bake, it can be a superset of it.

@ndeloof
Copy link
Contributor Author

ndeloof commented May 19, 2022

there shouldn't be duplicate implementation

I agree there should not be duplicate implementation to actually run the build process, but there also should not be duplicate implementations to parse a set of compose files, variables, local files etc into a compose model. While having compose to delegate the build execution to bake using latter's native format makes sense to me, having bake to support compose file format is wrong, as this prevent us to move forward with the compose specification without strict coordination between those two implementations - otherwise a valide compose file with fresh new attribute introduced in the compose specification could just not be built by bake!

I don't understand why this conversion code would be needed

.. so that bake doesn't have to parse (again) the compose file we just parsed, validated, interpolated. Also, there's not just compose build to be implemented. compose up or the incoming compose watch commands implies some build process with a subset of project services.

if you convert then you generate json, hcl is for humans

noted

From the compose UX side I think this repo should still have full control of that

I also think so. While it made sense for bake to support compose file, especially as compose v1 in python made it impossible to integrate buildx without pain, with compose v2 and native buildx/buildkit integration this makes lower sense, and we now have two ways to manage builds for a compose file. I wish we get compose build flexible enough so that switching to bake for the build is not required for users.

@tonistiigi
Copy link
Member

compose up or the incoming compose watch commands implies some build process with a subset of project services.

Yes for compose up that is not a build feature but "watch" shows the current problem. Looking at #9449 compose shouldn't add its own versions of additional build features. That PR does not understand what files are used by the build and what should be watched and what should not. It does not load the dockerignore file with same path rules that Dockerfile frontend does. Obviously, it doesn't work at all with any other frontends defined with #syntax that define their own build context and ignore rules. The only thing it can do is try to (incorrectly) replicate a feature of one specific version of one specific frontend. Even for Dockerfile, if any of this changes in the future version of the frontend (in past releases we have updated both dockerignore path rules and wildcard rules) it would break as well.

All these cases are solvable but need to be implemented in the correct layer that then compose can call into. In this case, the correct layer is the BuildKit component that transfers the correct local files based on the frontend's request. This layer can also set up a watcher for the paths that it has sent to the build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants