Support bash-style ARG substitution

[codefromthecrypt]

Description
Right now, it is a bit crufty to deal with substitutions of args. Ex $TARGETARCH -> s/amd64/X64 or even s/arm64/ARM64. It seems we could be better served if a limited version of bash style env substitution was allowed.

ENV GOROOT_${TARGETARCH//amd64/AMD64} ...

[tonistiigi]

This is similar to #1772

I'm starting to be in favor of #1772 but the problem with this one is that the replacement syntax is not part of posix but bash specific. And if you look at bash definition https://www.gnu.org/software/bash/manual/bash.html#Shell-Parameter-Expansion then there are a million other rules and we definitely can not implement all of them.

I guess it might still be fine to add only this pattern from bash and document it as such but we need to support all posix patterns first as well.

Is there a golang library that provides shell-style pattern matching? Because the current wildcard matching with path.Match is not quite the same and in here we should be compatible with shell.

I don't see a big issue of accepting this and #1772 to labs channel at least.

FYI @thaJeztah

[crazy-max]

but the problem with this one is that the replacement syntax is not part of posix but bash specific.

This lib seems to be able to handle bash replacements and expansion but there are some caveats.

[zchee]

or, https://github.com/drone/envsubst ?

[thaJeztah]

Generally, I'd prefer sticking to POSIX as well, but if there's good/valid use-cases, we could of course expand.

This lib seems to be able to handle bash replacements and expansion but there are some caveats.
or, https://github.com/drone/envsubst ?

There's some pros and cons to using a generic library;

• ✅ could save some work reinventing the wheel
• ❓ are they written with the same conditions in mind as a Dockerfile? Substitution has support for various "special" variables (see 2.5.2 Special Parameters and variables2.5.3 Shell Variables. (Not all of) those may be supported in context of a Dockerfile, so may have to be treated different. (I know Docker Compose is currently struggling with changes in .env parsing because they switched to a library, and there were many corner-cases in the original implementation that were slightly different than the new implementation, causing various regressions)
• ⚠️ Ideally, we keep control over the Dockerfile syntax, and curate the list of what's supported and what not; using a library imposes the risk that new syntaxes/features are implicitly introduced to the Dockerfile syntax. We may not have advertised those (or unaware they were added), but users may have discovered them, and now we will have to support them. Not necessarily problematic in all cases, but could be.

[zchee]

@thaJeztah Ah, I just found related package and shared it same as @crazy-max.
And, @tonistiigi said before, I also hit a case like below:

ARG LLVM_VERSION
ENV LLVM_VERSION_MAJOR=${LLVM_VERSION%%.*}

actually, I want

$ LLVM_VERSION=12.0.1; echo ${LLVM_VERSION%%.*}
12

but I agreed your concern.

I know Docker Compose is currently struggling with changes in .env.

using a library imposes the risk that new syntaxes/features are implicitly introduced to the Dockerfile syntax

Actually, I was thinking of fix the shell parser in the buildkit itself (frontend/dockerfile/shell) for support above behavior until I found this issue thread. Because, same as your concerns, We may lose control of the syntax of the Dockerfile if shell parser depends on any packages.

However, I found that is not Bourne Shell feature, GNU Bash expansions. then, I stopped sending PR because it is not a POSIX standard.

Bash includes the POSIX pattern removal ‘%’, ‘#’, ‘%%’ and ‘##’ expansions to remove leading or trailing substrings from variable values.

• https://www.gnu.org/software/bash/manual/html_node/Major-Differences-From-The-Bourne-Shell.html

---

As a result, My opinion is to support only the POSIX standard. Because writing a parser that is fully compatible with GNU Bash is too hard, it's not buildkit main feature, and i's overkill to put to in the buildkit. (Well, I'm not a buildkit maintainer, sorry for commented my opinion :P)

[jedevc]

I could possibly be interested in helping work on this 👀 It definitely looks like it would be around the same parts of code I've been looking at before.

I think if we wanted POSIX support (see 2.6.2 of the spec) we'd probably want to support the following:

• ${parameter%[word]} and ${parameter%%[word]}
• ${parameter#[word]} and ${parameter##[word]}
• ${parameterXdefault} and ${parameter:Xdefault}, where: X is one of (-, =, +, ?)

We might also want to steal the following from bash/zsh, which is what users might expect:

• ${#parameter} to give the length of parameter]
• ${parameter//[word]//[new_word]}

It would probably be relatively straightforward to implement most of these, just dropping them straight into the lexer with the others (probably some small amount of refactoring involved too). However, once implemented, they need maintaining, so that's probably a trade-off to make.

I'm also a fan of having it directly built into buildkit, instead of using an external library.

• Edge cases like ? require displaying an error if the parameter is unset - we probably want that nicely integrated with buildkit errors.
• Much tighter control over exact features to include/exclude.

If that sounds reasonable, I'm happy to implement a basic prototype of the POSIX support (and maybe the bash-specific ones as well, to see how they'd fit in), so we could judge from there.

[tonistiigi]

@jedevc

${parameterXdefault} - these should be already supported I think.

For the rest, I think the more complicated part is that [word] is a pattern, not just a constant. And that pattern has shell semantics that is not quite the same as wildcards in go. I don't think we have anything in the current implementation that understands these patterns.

For the posix ones LGTM to implement them.

For the rest, I'm not sure if the ${# is really that useful and would like to see some examples. The replace pattern is indeed a useful one.