Support RUN command arguments (mounts, network, security)

[MisterDA]

This is a first attempt at supporting RUN command arguments. I've added types for mounts and networks which are in BuildKit, and security, which is not yet in BuildKit. I've added an escape hatch to RUN as the ?args:string list parameter for later additions (and podman compat?). The security and network types, being relatively simple now, get their own parameters to RUN, but the mount values are serialized to string first.
The crunch function will not crunch shell scripts if their network or security differ, specified or not.
cc #137

[MisterDA]

@edwintorok I had not read your proposal carefully enough, do you already have code to achieve this? What do you think of this proposal?

[edwintorok]

My code is currently here master...edwintorok:ocaml-dockerfile:caching. I'm not entirely happy with how the API on that looks, and still evolving it as I use it.

[edwintorok]

Crunching mounts: I'm undecided what the best way forward is. Crunching commands with different mounts, especially with 'sharing=locked' might result in the build holding the lock for way longer than intended (small individual commands may be faster here even if they create additional layers).
Perhaps there should be an option to crunch to specify whether RUN lines with different mounts can be crunched together or not (in my changes so far I defaulted to no, and in your changes you defaulted to yes, so if we make it configurable it'll work for both of us).

[edwintorok]

@edwintorok I had not read your proposal carefully enough, do you already have code to achieve this? What do you think of this proposal?

I think I can rebase my changes on top of yours, in some areas they are more complete than mine.
Sorry if I made you duplicate some of what I already done (perhaps I should've opened a draft PR sooner).

I have a few more things on my branch (caching support in opam package manager, but that builds on top of this, and some doc comments, and a convenient value for the correct parsing_directive to use).

[MisterDA]

Crunching is already super fragile: is assumes a Unix shell-like syntax. I think we had an issue for something like this but I can't remember where it is. Anyway, I don't use crunch too much in Windows code, it behaves poorly with line continuations, &&, between cmd, powershell, Cygwin's shell...
It's doable to have a configurable switch to commands with different mounts, but I fear crunch would do too much. Perhaps it should error instead?

I have a few more things on my branch (caching support in opam package manager, but that builds on top of this, and some doc comments, and a convenient value for the correct parsing_directive to use).

Cool!

[edwintorok]

as long as the mounts are identical I think crunch should accept it (and be careful so we end up just with one set of mounts), otherwise either eject it or silently skip over it. Rejecting sounds better because it wouldn't break user expectations.

Yes crunching is already quite fragile if you e.g. 'cd' in one RUN line then the next one will run in a different directory. That may or may not be what the user wanted. It is useful to be able to run on small portions of the dockerfile, but when mounts are used I don't think that running it on the entire one would be useful.

[MisterDA]

I've simplified the code, I think. I've added your buildkit_syntax value too.
run and run_exec take well typed arguments.
crunch will raise if it tries to merge lines using different networks or security policies. It'll merge mounts if they are strictly identical.

[edwintorok]

Thanks, I have some additional code to use this, I'll rebase it on top of your latest branch.

[edwintorok]

run-args...edwintorok:ocaml-dockerfile:caching-run-args

There are some commits there that you may want to cherry-pick into this PR:
2dcbb8e Dockerfile: escape spaces in mount args
45dac5c Dockerfile.mli: more documentation on mounts
fe66805 Dockerfile: refer to {!buildkit_syntax}

In particular escaping is important because 'space' is otherwise a separator between mount args, but it may also show up inside mount args (e.g. a cache id, or a directory path, though in general on Linux paths don't have spaces that is not a reason to not implement escaping).

And I think the buildkit_syntax commit from above would also fix the ocaml-lint failure that ocaml-ci is complaining about here.

I've also written a test that generates a single dockerfile with all opam containers and builds them in parallel, but I'll open a separate PR with that (it is in my branch above), and the rest of the changes (to use caching for apt/yum/opam commands/etc.)

[MisterDA]

I've also written a test that generates a single dockerfile with all opam containers and builds them in parallel, but I'll open a separate PR with that (it is in my branch above), and the rest of the changes (to use caching for apt/yum/opam commands/etc.)

The caching part is super cool, that would be a great feature IMO.
As a matter of fact, there's already a test outputting all the build instructions to give us a point of reference when they change; it's just not in this repo. See ocurrent/docker-base-images/builds.expected.

Thank you for the other commits, I've cherry-picked them (rebasing with the format fix, which changed the commit id).

I'm all in for escapes, so I've included your patch.

[tmcgilchrist]

Additional tests would be most welcome, especially on the escaping spaces.

…

On Sat, 18 Feb 2023 at 4:33 am, Török Edwin ***@***.***> wrote: run-args...edwintorok:ocaml-dockerfile:caching-run-args <run-args...edwintorok:ocaml-dockerfile:caching-run-args> There are some commits there that you may want to cherry-pick into this PR: 2dcbb8e <2dcbb8e> Dockerfile: escape spaces in mount args 45dac5c <45dac5c> Dockerfile.mli: more documentation on mounts fe66805 <fe66805> Dockerfile: refer to {!buildkit_syntax} In particular escaping is important because 'space' is otherwise a separator between mount args, but it may also show up inside mount args (e.g. a cache id, or a directory path, though in general on Linux paths don't have spaces that is not a reason to not implement escaping). I've also written a test that generates a single dockerfile with all opam containers and builds them in parallel, but I'll open a separate PR with that (it is in my branch above). — Reply to this email directly, view it on GitHub <#139 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABJXOMCCA6OAPPILRQRK3TWX6ZAJANCNFSM6AAAAAAUPDJ7MI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[tmcgilchrist]

This looks great. Thank you.

…

On Tue, 21 Feb 2023 at 9:01 am, Tim McGilchrist ***@***.***> wrote: Additional tests would be most welcome, especially on the escaping spaces. On Sat, 18 Feb 2023 at 4:33 am, Török Edwin ***@***.***> wrote: > run-args...edwintorok:ocaml-dockerfile:caching-run-args > <run-args...edwintorok:ocaml-dockerfile:caching-run-args> > > There are some commits there that you may want to cherry-pick into this > PR: > 2dcbb8e > <2dcbb8e> > Dockerfile: escape spaces in mount args > 45dac5c > <45dac5c> > Dockerfile.mli: more documentation on mounts > fe66805 > <fe66805> > Dockerfile: refer to {!buildkit_syntax} > > In particular escaping is important because 'space' is otherwise a > separator between mount args, but it may also show up inside mount args > (e.g. a cache id, or a directory path, though in general on Linux paths > don't have spaces that is not a reason to not implement escaping). > > I've also written a test that generates a single dockerfile with all opam > containers and builds them in parallel, but I'll open a separate PR with > that (it is in my branch above). > > — > Reply to this email directly, view it on GitHub > <#139 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AABJXOMCCA6OAPPILRQRK3TWX6ZAJANCNFSM6AAAAAAUPDJ7MI> > . > You are receiving this because you are subscribed to this thread.Message > ID: ***@***.***> >