universe: go: add git by default in go.#Image

[vdemeester]

This adds git to the default go.#Image image so that 1.18 "just
works".

Fixes #1965

Signed-off-by: Vincent Demeester vincent@sbr.pm

[helderco]

I'd rather follow up on docker-library/golang#415, and if you really need git, then you can:

img: go.#Image & {
    packages: git: {}
}

[TomChv]

Thank you for the contribution, I've suggest a small improvement to avoid code duplication ;)

[vdemeester]

I'd rather follow up on docker-library/golang#415, and if you really need git, then you can:

img: go.#Image & {
    packages: git: {}
}

So In general, I would say yes. The only "trouble" with that is that go 1.18 assumes git is there by default. See #1965 for the error. Maybe installing git should be done conditionally on 1.18 version only ?

[helderco]

There's other options, like setting buildvcs=false by default.

[helderco]

There's a push in golang/go#51748 to detect this situation and deal accordingly:

Require a VCS tool for -buildvcs=true, and implicitly disable stamping when the tool is missing for -buildvcs=auto

So if this goes through we don't have to change anything.

In the meantime we can add:

flags: "-buildvcs": *false | true

Note that previous docker images (1.17) didn't include the git binary, it's just this new default behavior in go 1.18 about adding vcs info to the build. This is why I think this is the proper fix. And if the default gets fixed upstream, we can then remove this flag.

[helderco]

Also @vdemeester, if you're up for it, add a test which includes a git init'ed source dir to check if the build doesn't fail because of it.

[vdemeester]

@helderco I'll do this tomorrow 👼🏼 😉

[aluzzardi]

👍 on this. I just hit the problem myself, had to do a similar workaround.

Ideally the upstream go image would already include git, but if that doesn't come soon enough, we should go ahead with this

[shykes]

I think we should add the polyfill without waiting for upstream. We can always remove it later if it becomes redundant.

[vdemeester]

I probably should add a 2nd test entry here (one with, one without the flag), right ?

[vdemeester]

Added the FIXME 🙃

[helderco]

Yes, keep this, but it's "...image includes git". Might as well reference the issues: Remove once golang image includes git (docker-library/golang#415) **or** go compiler is smart with -buildvcs (golang/go#51748).

[helderco]

There's two levels to this:

1. Missing package in 1.18 docker-library/golang#415
   Will the upstream golang docker image include git? Doesn't seem likely, they are waiting on the status of 👇

2. cmd/go: add (and default to) -buildvcs=auto golang/go#51748
   Will golang itself disable buildvcs by default in case git isn't in $PATH? Very possible 👇

That sounds like a consensus to me.

Option (3) it is! 👍

Originally posted by @bcmills in golang/go#51748 (comment)

---

So I'm partial to adding a shim (i.e., follow golang's lead), which means: don't add git, and set -buildvcs=false if it's not installed in the image.

1. If golang ends up defaulting to -buildvcs=auto, then it's easy to remove the shim;
2. If golang stays the course, then we either keep the convenience or get on board and remove the shim just the same.

[helderco]

There's an argument for making dagger have the convenience of adding git on top of the golang docker image.

Pros:

• Easiest fix, at least for now;
• More convinent for the most common use case;

Cons

• Harder to remove later if users come to depend on it, then we're the ones breaking stuff;
• You may be using mercurial or something else and we're back to square 1, but then you need to be aware of this change from the go language and either install your tool or add the flag;

[helderco]

Looking at your change I come to realize that our definition needs a better way to set flags. This can come in another PR.

From my previous comment, I say keep it simple for now:

1. Just add git to the image;
2. Don't set the flag, we'll allow for better user control soon;

[helderco]

Ping @shykes

[vdemeester]

cc @aluzzardi

[shykes]

This seems like a useful short-term fix. We will not be able to remove git from the default image in this package, because as @helderco noted, that might break developers that depend on it. But I think it's fine to keep git installed forever. Remember that this is only for the default image. Developers can easily swap in their own image, with or without git.

[shykes]

lgtm

[shykes]

We're good to merge @helderco

[helderco]

It's blocked until @TomChv approves his requested changes.

[aluzzardi]

@TomChv's changes were addressed -- merging

Changes addressed

[helderco]

https://go-review.googlesource.com/c/go/+/398855/

[vdemeester]

@helderco will probably be fixed in 1.18.1 or something I guess ? 🤔