buildctl: new CLI ("Option C+") #807

AkihiroSuda · 2019-01-31T15:41:23Z

See #807 (comment)

Signed-off-by: Akihiro Suda [email protected]

tonistiigi

I'm not 100% sure its a good idea but maybe we could support single value --frontend=dockerfile.v0, --export=oci as well if value is not csv.

cmd/buildctl/build.go

tonistiigi · 2019-02-01T22:12:52Z

cc @tiborvass @thaJeztah

tiborvass · 2019-02-01T22:26:45Z

Bikeshed warning: I think I find type weird in this case. It's not a frontend type it's the name of the frontend image. So name, image, ... ?

README.md

thaJeztah · 2019-02-01T22:38:56Z

Good point (type vs name). I'm a bit cautious with using image= for that, as (I think) that should be an implementation detail (buildkit happens to use an image for distribution)

tonistiigi · 2019-02-01T22:47:58Z

More options:

leave frontend with 2 flags, with --frontend-opt accepting csv
make all fields to take name,<csv> format instead. eg --frontend dockerfile.v0,target=foo, --export oci, --export image,name=foo

thaJeztah · 2019-02-01T23:10:54Z

leave frontend with 2 flags, with --frontend-opt accepting csv

For frontend this would possibly still work; I don't think multiple frontends can be used on a single build? (unless I'm forgetting something).

For exporter we must group them together in a single flag.

make all fields to take name,<csv> format instead

I think (should check) that in docker we made an exception for the first parameter to be positional (at least we talked about it), so:

First param uses =, so "positional" rule does not take effect. type does therefore not have to be the first option (in this case, it's set last)

--exporter opt=foo=bar,opt=bla=bla,type=image

First parameter does not have a =, so the "positional" rule takes effect: first parameter is type

--exporter image,opt=foo=bar,opt=bla=bla,type=image

Invalid: both first (positional), and last (type=) option specify a type

--exporter image,opt=foo=bar,opt=bla=bla,type=image

tonistiigi · 2019-02-01T23:19:00Z

First param uses =, so "positional" rule does not take effect. type does therefore not have to be the first option (in this case, it's set last)

The main problem atm (in addition to this being cumbersome and hard to name) is the point you brought up that type could actually be a key that the frontend uses. Also, this "primary" positional value is required in all cases and only having options with no type/name never makes sense. So if we would do this, I'd make it a requirement and only way to define the value (in addition to the hidden compatibility with old syntax).

tonistiigi · 2019-02-04T19:47:05Z

Let's also make sure that there is a sane way to do comma separated values as they are quite common (platform,name,cache-from) and add examples for how to do it.

AkihiroSuda · 2019-02-13T15:51:22Z

Let's also make sure that there is a sane way to do comma separated values

type=foo,"opt=opt1,opt2,opt3" ? (Although we might want to remove type=)
https://play.golang.org/p/R_p_syvcX31

AkihiroSuda · 2019-02-13T15:57:00Z

Another options is to rather deprecate CSV and restore the legacy form (sorry for going back and forth), with support for multiple (e.g.) exporters, but not sure how it can be implemented with any existing CLI library.

--exporter image --exporter-opt name=docker.io/username/image1,docker.io/username/image2 --exporter oci --exporter-opt output=/path/to/tar

tonistiigi · 2019-02-13T19:03:17Z

Any counterarguments for #807 (comment) ?

...,"opt=opt1,opt2,opt3"

I think this is acceptable if it's documented.

tiborvass · 2019-02-13T23:38:59Z

I'm fine with the proposal, but just so everyone is on the same page:
type=foo,"opt=opt1,opt2,opt3" means that you would have to type 'type=foo,"opt=opt1,opt2,opt3"' in a shell to escape the double quotes ".

tonistiigi · 2019-02-13T23:40:44Z

@tiborvass or escape them

thaJeztah · 2019-02-14T00:54:51Z

So the format would support both

type=foo,"opt=opt1,opt2,opt3"

and

type=foo,opt=opt1,opt=opt2,opt=opt3

(both being equivalent)?

tonistiigi · 2019-02-14T00:59:07Z

type=foo,opt=opt1,opt=opt2,opt=opt3

We could support both with CLI magic but the second one would just convert to the first one. In API it is a comma separated string.

The proposed format is:

foo,bar=baz,"opt=opt1,opt2,opt3"

AkihiroSuda · 2019-02-18T04:34:58Z

Also, this "primary" positional value is required in all cases and only having options with no type/name never makes sense. So if we would do this, I'd make it a requirement and only way to define the value (in addition to the hidden compatibility with old syntax).

How should it work with --export-cache and --import-cache? (#615)

If we focus on compatibility, the flag would be like --export-cache localhost:5000/myrepo:buildcache,type=registry, but I'm wondering it looks inconsistent to have type= in non-primary key-value.

tonistiigi · 2019-02-18T04:37:10Z

How should it work with --export-cache and --import-cache?

--export-cache=registry,ref=localhost:5000/myrepo:buildcache

AkihiroSuda · 2019-02-18T05:14:33Z

Off-topic: do we also want to remove type= from RUN --mount and docker run/service --mount?

tonistiigi · 2019-02-18T05:31:33Z

Off-topic: do we also want to remove type= from RUN --mount and docker run/service --mount?

I don't think so. It would be good to figure out syntax for docker build though.

docker build -o ./out .
docker build -o - . | tar xv
docker build -o files,dest=output .
docker build -o tarball,dest=- .
docker build -o tarball,dest=out.tgz .
docker build -o registry,name=ref . # not required before distributed build
docker build -o oci-image,name=ref . # after containerd
docker build -o cache-registry,name=ref . # after containerd
docker build -o cache-local,name=ref . # after containerd
docker build --cache-from ref
docker build --cache-from local,dir=/foo

?

Maybe in docker it makes more sense to explicitly require type= on the longform values.

There is still an option to not do the positional value in buildctl and leave frontend as an exception. This isn't super bad as frontend is also the only one where it doesn't make sense to have an array of values.

AkihiroSuda · 2019-02-18T05:34:48Z

I'll try to update PR to support both --frontend FRONTEND and --frontend type=FRONTEND

tonistiigi · 2019-02-18T05:39:38Z

@AkihiroSuda

And for opt? I don't think we should do --frontend type=FRONTEND,foo=bar. It doesn't fit the actual API data types. We could do it for the other flags though.

AkihiroSuda · 2019-02-18T06:06:22Z

To sum up:

Should be supported

--frontend dockerfile.v0,target=foo
--export-cache registry,ref=localhost:5000/myrepo:buildcache (same applies to --import-cache)
--export-cache type=registry,ref=localhost:5000/myrepo:buildcache
--export docker,name=myimage
--export docker,"names=myimage1,myimage2" (can be separate PR)
--secret id=secretname,src=filepath
--secret file,id=secretname,src=filepath
--secret type=file,id=secretname,src=filepath

Should be supported but deprecated

--frontend dockerfile.v0 --frontend-opt target=foo
--export-cache localhost:5000/myrepo:buildcache
--exporter docker --exporter-opt name=myimage
--exporter docker --exporter-opt names=myimage1,myimage2

Should NOT be supported

--frontend type=dockerfile.v0,target=foo (should print a warning like "frontend missing, you meant --frontend dockerfile.v0,target=foo?")
--export-cache registry

Let me know if this is not correct.

tonistiigi · 2019-02-18T06:45:02Z

I think it's confusing to support multiple variants together.

Option A

Should be supported

--frontend dockerfile.v0,target=foo
--export-cache registry,ref=localhost:5000/myrepo:buildcache (same applies to --import-cache)
--export docker,name=myimage
--export docker,"names=myimage1,myimage2" (can be separate PR)
--secret id=secretname,src=filepath
--secret file,id=secretname,src=filepath

Should be supported but deprecated

--export-cache type=registry,ref=localhost:5000/myrepo:buildcache (never existed on a release)
--secret type=file,id=secretname,src=filepath
--frontend dockerfile.v0 --frontend-opt target=foo
--export-cache localhost:5000/myrepo:buildcache
--exporter docker --exporter-opt name=myimage
--exporter docker --exporter-opt names=myimage1,myimage2

Should NOT be supported

--frontend type=dockerfile.v0,target=foo (should print a warning like "frontend missing, you meant --frontend dockerfile.v0,target=foo?")
--export-cache registry

Option B

Should be supported

--frontend dockerfile.v0
--frontend-opts target=foo,platform=linux/arm
--export-cache registry,ref=localhost:5000/myrepo:buildcache (same applies to --import-cache)
--export docker,name=myimage
--export docker,"names=myimage1,myimage2" (can be separate PR)
--secret id=secretname,src=filepath
--secret file,id=secretname,src=filepath

Should be supported but deprecated

--export-cache type=registry,ref=localhost:5000/myrepo:buildcache (never existed on a release)
--secret type=file,id=secretname,src=filepath
--frontend dockerfile.v0 --frontend-opt target=foo (could also be just supported)
--export-cache localhost:5000/myrepo:buildcache
--exporter docker --exporter-opt name=myimage
--exporter docker --exporter-opt names=myimage1,myimage2

Should NOT be supported

--frontend type=dockerfile.v0,target=foo (should print a warning like "frontend missing, you meant --frontend dockerfile.v0,target=foo?")
--export-cache registry

Option C

Should be supported

--frontend dockerfile.v0
--frontend-opts target=foo,platform=linux/arm
--export-cache type=registry,ref=localhost:5000/myrepo:buildcache
--export type=docker,name=myimage
--export type=docker,"names=myimage1,myimage2" (can be separate PR)
--secret type=file,id=secretname,src=filepath
--secret id=secretname,src=filepath (could be supported with default type value)

Should be supported but deprecated

--frontend dockerfile.v0 --frontend-opt target=foo (could also be just supported)
--export-cache localhost:5000/myrepo:buildcache
--exporter docker --exporter-opt name=myimage
--exporter docker --exporter-opt names=myimage1,myimage2

Should NOT be supported

--export-cache registry,ref=localhost:5000/myrepo:buildcache (same applies to --import-cache)
--frontend type=dockerfile.v0,target=foo (should print a warning like "frontend missing, you meant --frontend dockerfile.v0,target=foo?")
--export-cache registry

See moby#807 (comment) Close moby#774 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2019-03-01T07:32:47Z

Update PR to use --export-cache type=local,dest=/path/to/dir --import-cache type=registry,src=example.com/foo. RFC.

tonistiigi · 2019-03-01T19:33:00Z

It's not too important but I'd rather leave the src/dest only to values where it points to a file/path and leave it to ref if it is a reference.

AkihiroSuda · 2019-03-02T06:27:50Z

@thaJeztah @tiborvass @ijc WDYT?

thaJeztah · 2019-03-04T14:52:45Z

README.md

@@ -122,7 +122,7 @@ During development, Dockerfile frontend (dockerfile.v0) is also part of the Buil

 ```
 buildctl build --frontend=dockerfile.v0 --local context=. --local dockerfile=.
-buildctl build --frontend=dockerfile.v0 --local context=. --local dockerfile=. --frontend-opt target=foo --frontend-opt build-arg:foo=bar
+buildctl build --frontend=dockerfile.v0 --local context=. --local dockerfile=. --opt target=foo --opt build-arg:foo=bar


Bit on the fence on the --frontend-opt -> --opt change, as it's not clear to me that they are related to each other. 😅

Why should you as a user care if they are related or not?

To understand that an option is related to the frontend, not to (e.g.) the --output.

If we drop the prefix, should we also drop the --export- prefix from --export-cache?

I wouldn't say they are strictly related to the frontend. They are more like options for the build request, while the other options are for a specific component (exporter, cache) or are cli side (local, secret).

Ah; gotcha - guess I got confused because they previously were prefixed

AkihiroSuda · 2019-03-05T02:27:50Z

〉leave it to ref if it is a reference.

Could you be more specific about the reason for this?

tonistiigi · 2019-03-05T04:16:27Z

@AkihiroSuda I'm quite sure that we can't make it into a rule that every exporter/importer will only have a src/dest option in the future. That's why its a generic map in the API. For paths, src/dest seems obvious, for refs I've not seen it before. I'm willing to change my mind though if people think the other logic makes more sense.

See moby#807 (comment) Close moby#774 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2019-03-05T04:36:20Z

reverted --import-cache type=registry,src=... --export-cache type=registry,dest=...

See moby#807 (comment) Close moby#774 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2019-03-06T04:20:57Z

rebased

AkihiroSuda · 2019-03-07T01:07:35Z

Any task left?

tonistiigi · 2019-03-08T18:59:04Z

@tiborvass @thaJeztah @ijc

thaJeztah

LGTM, thanks!

See moby/buildkit#807 (comment) Close #774 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the csv-frontend branch from 7624b91 to 808b96f Compare January 31, 2019 15:52

AkihiroSuda changed the title ~~buildctl: migrate --frontend to CSV style~~ buildctl: CSV-style --frontend and --export Jan 31, 2019

AkihiroSuda mentioned this pull request Jan 31, 2019

buildctl: Use CSV style for --frontend and --export #774

Closed

tonistiigi added this to the v0.4.0 milestone Feb 1, 2019

AkihiroSuda force-pushed the csv-frontend branch 2 times, most recently from 57aefd9 to 7a263e6 Compare February 1, 2019 09:55

tonistiigi reviewed Feb 1, 2019

View reviewed changes

cmd/buildctl/build.go Outdated Show resolved Hide resolved

cmd/buildctl/build.go Outdated Show resolved Hide resolved

thaJeztah reviewed Feb 1, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

AkihiroSuda force-pushed the csv-frontend branch from 0df646b to 5d970dc Compare March 1, 2019 02:33

AkihiroSuda added a commit to AkihiroSuda/buildkit_poc that referenced this pull request Mar 1, 2019

buildctl: new CLI ("Option C+")

5d970dc

See moby#807 (comment) Close moby#774 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the csv-frontend branch from 5d970dc to fa15e25 Compare March 1, 2019 02:50

AkihiroSuda added a commit to AkihiroSuda/buildkit_poc that referenced this pull request Mar 1, 2019

buildctl: new CLI ("Option C+")

fa15e25

See moby#807 (comment) Close moby#774 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda removed the status/do-not-merge label Mar 1, 2019

AkihiroSuda force-pushed the csv-frontend branch from fa15e25 to b542bd0 Compare March 1, 2019 07:29

AkihiroSuda added a commit to AkihiroSuda/buildkit_poc that referenced this pull request Mar 1, 2019

buildctl: new CLI ("Option C+")

f18314f

See moby#807 (comment) Close moby#774 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the csv-frontend branch from b542bd0 to c587dfc Compare March 1, 2019 07:32

AkihiroSuda force-pushed the csv-frontend branch from c587dfc to 1f133d4 Compare March 1, 2019 09:48

thaJeztah reviewed Mar 4, 2019

View reviewed changes

AkihiroSuda force-pushed the csv-frontend branch from 1f133d4 to a84403a Compare March 5, 2019 04:36

AkihiroSuda added a commit to AkihiroSuda/buildkit_poc that referenced this pull request Mar 5, 2019

buildctl: new CLI ("Option C+")

a84403a

See moby#807 (comment) Close moby#774 Signed-off-by: Akihiro Suda <[email protected]>

buildctl: new CLI ("Option C+")

5c9f7b8

See moby#807 (comment) Close moby#774 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the csv-frontend branch from a84403a to 5c9f7b8 Compare March 6, 2019 04:20

tonistiigi approved these changes Mar 8, 2019

View reviewed changes

tiborvass approved these changes Mar 8, 2019

View reviewed changes

thaJeztah approved these changes Mar 8, 2019

View reviewed changes

tiborvass merged commit 2a857a3 into moby:master Mar 8, 2019

AkihiroSuda mentioned this pull request Mar 12, 2019

Progress is redirected to output on Travis-CI #863

Closed

tonistiigi mentioned this pull request May 12, 2020

multiple export point #1485

Closed

crazy-max pushed a commit to crazy-max/dockerfile that referenced this pull request Jan 8, 2022

buildctl: new CLI ("Option C+")

b9f2a07

See moby/buildkit#807 (comment) Close #774 Signed-off-by: Akihiro Suda <[email protected]>

buildctl: new CLI ("Option C+") #807

buildctl: new CLI ("Option C+") #807

Conversation

AkihiroSuda commented Jan 31, 2019 • edited Loading

See #807 (comment)

tonistiigi left a comment

Choose a reason for hiding this comment

tonistiigi commented Feb 1, 2019

tiborvass commented Feb 1, 2019

thaJeztah commented Feb 1, 2019

tonistiigi commented Feb 1, 2019

thaJeztah commented Feb 1, 2019

tonistiigi commented Feb 1, 2019

tonistiigi commented Feb 4, 2019

AkihiroSuda commented Feb 13, 2019

AkihiroSuda commented Feb 13, 2019

tonistiigi commented Feb 13, 2019

tiborvass commented Feb 13, 2019

tonistiigi commented Feb 13, 2019

thaJeztah commented Feb 14, 2019

tonistiigi commented Feb 14, 2019

AkihiroSuda commented Feb 18, 2019

tonistiigi commented Feb 18, 2019

AkihiroSuda commented Feb 18, 2019

tonistiigi commented Feb 18, 2019 • edited Loading

AkihiroSuda commented Feb 18, 2019

tonistiigi commented Feb 18, 2019 • edited Loading

AkihiroSuda commented Feb 18, 2019

Should be supported

Should be supported but deprecated

Should NOT be supported

tonistiigi commented Feb 18, 2019

Option A

Should be supported

Should be supported but deprecated

Should NOT be supported

Option B

Should be supported

Should be supported but deprecated

Should NOT be supported

Option C

Should be supported

Should be supported but deprecated

Should NOT be supported

AkihiroSuda commented Mar 1, 2019

tonistiigi commented Mar 1, 2019

AkihiroSuda commented Mar 2, 2019

thaJeztah Mar 4, 2019

Choose a reason for hiding this comment

tonistiigi Mar 4, 2019

Choose a reason for hiding this comment

thaJeztah Mar 4, 2019

Choose a reason for hiding this comment

tonistiigi Mar 4, 2019

Choose a reason for hiding this comment

thaJeztah Mar 8, 2019

Choose a reason for hiding this comment

AkihiroSuda commented Mar 5, 2019

tonistiigi commented Mar 5, 2019

AkihiroSuda commented Mar 5, 2019

AkihiroSuda commented Mar 6, 2019

AkihiroSuda commented Mar 7, 2019

tonistiigi commented Mar 8, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Jan 31, 2019 •

edited

Loading

tonistiigi commented Feb 18, 2019 •

edited

Loading

tonistiigi commented Feb 18, 2019 •

edited

Loading