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: simplify for modern Go #993

Merged
merged 2 commits into from
Apr 6, 2022

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Mar 30, 2022

Quite a few changes, mostly removing old stuff though.

  1. "GO111MODULE=off" is no longer required to be set by default (and
    it used to be overridden below anyway).

  2. "go build" no longer requires explicit "-mod=vendor", as this is the
    default since go 1.14.

  3. GOPROXY is set to proxy.golang.org by default since go 1.13.

  4. Always use default GOPATH (which is $HOME/go since Go 1.8; earlier
    releases needed to set it explicitly).

  5. Drop the code that handles multiple comma-separated GOPATH elements
    (when using modules, GOPATH is no longer used for resolving imports,
    which basically means using multiple paths is useless).

  6. Drop go-get macro (which is used to install md2man), use go install
    directly (supported since Go 1.16). While at it, do not check if
    go-md2man is available, install it unconditionally. This removes
    the need to have GOBIN make variable.

  7. Remove GOPKGBASEDIR and GOPKGBASEDIR, defined by Makefile, were never
    used.

Signed-off-by: Kir Kolyshkin [email protected]

@kolyshkin
Copy link
Contributor Author

close/reopen to restart ci

@kolyshkin kolyshkin closed this Mar 31, 2022
@kolyshkin kolyshkin reopened this Mar 31, 2022
@kolyshkin kolyshkin force-pushed the misc branch 2 times, most recently from 69ad0a8 to 1e2b319 Compare April 1, 2022 22:50
@kolyshkin kolyshkin marked this pull request as ready for review April 1, 2022 23:02
@kolyshkin
Copy link
Contributor Author

I have no idea why fedora-35 ci times out 😕

@saschagrunert
Copy link
Member

I have no idea why fedora-35 ci times out confused

The tests timed out, can you increase the timeout in cirrus and re-push again please?

Currently, Fedora CI job often times out after 30m.

Signed-off-by: Kir Kolyshkin <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Apr 5, 2022

LGTM
@containers/podman-maintainers PTAL
@saschagrunert PTAL

@jwhonce
Copy link
Member

jwhonce commented Apr 5, 2022

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 5, 2022

@jwhonce: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kolyshkin
Copy link
Contributor Author

Interesting; when I increased the timeout from 30m to 60m, it succeeded in 25m 😕 and previously it was always timing out.

@kolyshkin
Copy link
Contributor Author

In any case, 25m is dangerously close to 30m so let's bump it to 60m just in case (no one likes flaky CI).

Quite a few changes, mostly removing old stuff though.

1. "GO111MODULE=off" is no longer required to be set by default (and
   it used to be overridden below anyway).

2. "go build" no longer requires explicit "-mod=vendor", as this is the
   default since go 1.14.

3. GOPROXY is set to proxy.golang.org by default since go 1.13.

4. Always use default GOPATH (which is $HOME/go since Go 1.8; earlier
   releases needed to set it explicitly).

5. Drop the code that handles multiple comma-separated GOPATH elements
   (when using modules, GOPATH is no longer used for resolving imports,
   which basically means using multiple paths is useless).

6. Drop go-get macro (which is used to install md2man), use go install
   directly (supported since Go 1.16). While at it, do not check if
   go-md2man is available, install it unconditionally. This removes
   the need to have GOBIN make variable.

7. Remove GOPKGBASEDIR and GOPKGBASEDIR, defined by Makefile, were never
   used.

8. Rm PROJECT, use relative paths in test-unit target.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

Just added one more simplification. Since github does not show diffs between revisions, here is it:

diff --git a/Makefile b/Makefile
index 1d049685..11100ba9 100644
--- a/Makefile
+++ b/Makefile
@@ -4,7 +4,6 @@ BUILDTAGS := containers_image_openpgp,systemd,exclude_graphdriver_devicemapper
 DESTDIR ?=
 PREFIX := /usr/local
 CONFIGDIR := ${PREFIX}/share/containers
-PROJECT := github.com/containers/common
 
 define go-build
        CGO_ENABLED=0 \
@@ -93,8 +92,8 @@ test: test-unit
 test-unit:
        go test --tags $(BUILDTAGS) -v ./libimage
        go test --tags $(BUILDTAGS) -v ./libnetwork/...
-       go test --tags $(BUILDTAGS) -v $(PROJECT)/pkg/...
-       go test --tags remote,seccomp,$(BUILDTAGS) -v $(PROJECT)/pkg/...
+       go test --tags $(BUILDTAGS) -v ./pkg/...
+       go test --tags remote,seccomp,$(BUILDTAGS) -v ./pkg/...
 
 .PHONY: codespell
 codespell:

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@rhatdan
Copy link
Member

rhatdan commented Apr 6, 2022

/approve
/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 6, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kolyshkin, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants