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

Makefile: allow GOVERSION to be passed, but being empty #266

Merged
merged 3 commits into from
Feb 15, 2022

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Feb 15, 2022

Makefile: remove unused GOARCH variable

This variable was previously used to generate the BUILDER_IMAGE variable, but appears to be unused since commit 018f1b9 (#154)

Makefile: remove workaround for s390x GOLANG_IMAGE

This workaround was added in d1225f6 (#140), and from the related PR;

This is a workaround for golang:1.12.x images where the manifest list is missing
s390x. With golang:1.13.x or newer this if block can ve removed again.

Makefile: allow GOVERSION to be passed, but being empty

Some of our Jenkins jobs allow for GOVERSION to be set as parameter but
unconditionally pass the variable when calling make from this repository.
The intent of those parameters was to allow overriding the Go version when
building packages, but due to the parameter to be passed unconditionally,
curently requires us to always set the Go version to build, and making sure
it's set to the correct version for containerd to build.

Make (unfortunately) seems to make it nigh impossible to detect if a variable
is either unset (not passed) or empty, while also accounting for the variable
to be passed as FOO= make <target> and make FOO= <target> and allowing
the default value to be evaluated lazily (on "use"), which is needed in our
case (to get the default version from containerd's repository, which requires
the source code to be checked out first).

Variations of the below all failed (as demonstrated below);

ifndef FOO
FOO = $(shell echo "default1")
else ifeq ($(strip $(FOO)),)
FOO = $(shell echo "default2")
endif

.PHONY: test
test:
@echo foo is "$(FOO)" from $(origin FOO)
$ make test
foo is default1 from file

$ FOO= make test
foo is default1 from file

$ make FOO= test
foo is from command line

To work around this, we're introducing separate variables for the
GOVERSION (to be used to override the default) and GOLANG_VERSION,
which is the (lazily evaluated) variable used to construct the golang
image reference to use.

This variable was previously used to generate the `BUILDER_IMAGE` variable, but
appears to be unused since commit 018f1b9

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This workaround was added in d1225f6, and from
the related PR;

> This is a workaround for golang:1.12.x images where the manifest list is missing
> s390x. With golang:1.13.x or newer this if block can ve removed again.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Some of our Jenkins jobs allow for GOVERSION to be set as parameter but
unconditionally pass the variable when calling `make` from this repository.
The intent of those parameters was to allow overriding the Go version when
building packages, but due to the parameter to be passed unconditionally,
curently requires us to always set the Go version to build, and making sure
it's set to the correct version for containerd to build.

Make (unfortunately) seems to make it nigh impossible to detect if a variable
is either unset (not passed) or empty, while also accounting for the variable
to be passed as `FOO= make <target>` and `make FOO= <target>` _and_ allowing
the default value to be evaluated lazily (on "use"), which is needed in our
case (to get the default version from containerd's repository, which requires
the source code to be checked out first).

Variations of the below all failed (as demonstrated below);

```makefile
ifndef FOO
	FOO = $(shell echo "default1")
else ifeq ($(strip $(FOO)),)
	FOO = $(shell echo "default2")
endif

.PHONY: test
test:
	@echo foo is "$(FOO)" from $(origin FOO)
```

```bash
$ make test
foo is default1 from file

$ FOO= make test
foo is default1 from file

$ make FOO= test
foo is  from command line
```

To work around this, we're introducing separate variables for the
`GOVERSION` (to be used to override the default) and `GOLANG_VERSION`,
which is the (lazily evaluated) variable used to construct the golang
image reference to use.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

Sigh.. so the default repository that's configured in the images is using http://, but looks like the package-repositories have been changed to enforce TLS (https://), and for some reason that sometimes fails;

#23 22.99 http://vault.centos.org/centos/7/os/Source/repodata/repomd.xml: [Errno 14] HTTPS Error 301 - Moved Permanently
#23 22.99 Trying other mirror.
#23 22.99 failure: repodata/repomd.xml from base-source: [Errno 256] No more mirrors to try.
#23 22.99 http://vault.centos.org/centos/7/os/Source/repodata/repomd.xml: [Errno 14] HTTPS Error 301 - Moved Permanently
#23 ERROR: executor failed running [/bin/sh -c . /root/.rpm-helpers; install_build_deps SPECS/containerd.spec]: exit code: 1

@thaJeztah
Copy link
Member Author

Opened CentOS/sig-cloud-instance-images#191 for the above issue

@thaJeztah
Copy link
Member Author

@rumpl @StefanScherer ptal

Copy link
Member

@StefanScherer StefanScherer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit 8d47786 into docker:master Feb 15, 2022
@thaJeztah thaJeztah deleted the improve_golang_handling branch February 15, 2022 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants