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

Add support for Go Modules #2073

Merged
merged 7 commits into from
Mar 16, 2020
Merged

Add support for Go Modules #2073

merged 7 commits into from
Mar 16, 2020

Conversation

odinuge
Copy link
Contributor

@odinuge odinuge commented Jun 18, 2019

This removes vndr, and swiches to native Go Modules instead. All modules
are kept on the old version.

Keeps the vendor/ dir, so everything is backwards compatible.

This is more or less only go mod init

This would make it way easier for other projects using runc as dependency to keep track of the dependencies (and their version) that runc require.

It would be very nice to get this in before the v1.0.0 release!

@odinuge odinuge force-pushed the modules branch 4 times, most recently from 38676c6 to 4fc7a7c Compare June 18, 2019 18:47
@vrothberg
Copy link
Contributor

Just a drive-by-comment (I am no maintainer). To make sure that the code and the ./vendor directory are in sync, we are using the following checks in the github.com/containers projects:

Makefile target

vendor:
	export GO111MODULE=on \
		$(GO) mod tidy && \
		$(GO) mod vendor && \
                $(GO) mod verify

Shell script tree_status.sh

Checks if the git tree has changed and prints a report.

#!/usr/bin/env bash
set -e

STATUS=$(git status --porcelain)
if [[ -z $STATUS ]]
then
	echo "tree is clean"
else
	echo "tree is dirty, please commit all changes"
	echo ""
	echo "$STATUS"
	exit 1
fi

CI (e.g., Travis)

Both are called in a CI job (make vendor && ./hack/tree_status.sh) to avoid inconsistencies from being committed.

@odinuge
Copy link
Contributor Author

odinuge commented Jun 19, 2019

Hi @vrothberg! Thanks for the tips! k8s use the same too, but with a bit more hack-files. 😄

Added a simple check in 4f6014b now.

@odinuge odinuge force-pushed the modules branch 2 times, most recently from 2f0088d to f66aadc Compare June 19, 2019 20:55
@thaJeztah
Copy link
Member

similar discussion about validation in distribution/distribution#2941 (comment) (haven't checked if one has advantages over the other)

@vrothberg
Copy link
Contributor

similar discussion about validation in docker/distribution#2941 (comment) (haven't checked if one has advantages over the other)

Awesome!

@vrothberg
Copy link
Contributor

Hi @vrothberg! Thanks for the tips! k8s use the same too, but with a bit more hack-files. smile

Added a simple check in 4f6014b now.

LGTM, thanks!

@AkihiroSuda
Copy link
Member

needs rebase

@nathanjsweet
Copy link

nathanjsweet commented Aug 27, 2019

@vrothberg @odinuge do you need help to get this going? Definitely bother the devs on the slack channel to get this across the finish line. To any of the OCI devs: it's harder to contribute to, use, etc this repository without go modules.

@odinuge odinuge force-pushed the modules branch 4 times, most recently from ca160ae to 5919d16 Compare August 28, 2019 11:07
@odinuge
Copy link
Contributor Author

odinuge commented Aug 28, 2019

Thanks for the interest @nathanjsweet!

do you need help to get this going?
I think this should be good to go now (tests fail because of some problems installing postgres in CI), but I am absolutely open for suggestions. I have stopped rebasing the PR when conflicts occur in await for some interest from the contributors/reviewers from OCI. Hopefully some of them will look at it, so we can get go modules into the project.

Definitely bother the devs on the slack channel to get this across the finish line. To any of the OCI devs: it's harder to contribute to, use, etc this repository without go modules.

Agree! Have been using this in several projects myself, including k8s, and go modules (and a new release) will help the developer experience a lot!

@odinuge
Copy link
Contributor Author

odinuge commented Aug 28, 2019

I also think #2029 is kinda important, and that PR should be merged before this one.

@odinuge
Copy link
Contributor Author

odinuge commented Aug 28, 2019

ping @mrunalp

@saschagrunert
Copy link
Contributor

@odinuge can you please rebase on top of the latest master branch? Then I would give this one a review if you don't mind. :)

@odinuge odinuge force-pushed the modules branch 3 times, most recently from 09817b6 to e9d5615 Compare November 13, 2019 10:53
Makefile Outdated
$(GO) mod vendor && \
$(GO) mod verify

vendor-verify: vendor
Copy link
Member

Choose a reason for hiding this comment

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

this should be put to the validate target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Thanks for catching it. Updated the PR now.

@odinuge odinuge force-pushed the modules branch 2 times, most recently from 6f55ffc to cc22645 Compare November 13, 2019 13:19
@hqhq
Copy link
Contributor

hqhq commented Mar 6, 2020

LGTM

Use go modules makes sence to me, but I'm not sure why we are not using it in the first place, I think more maintainers should be aware of this change before merge.
@crosbymichael @mrunalp @cyphar

Approved with PullApprove

@h-vetinari
Copy link

@hqhq: [...] but I'm not sure why we are not using it in the first place [...]

Go modules became mature with Go 1.13 (released Sept '19), and this PR has been open since June 19. Together with the usual turnaround time for reviews here, that already seems like more than enough explanation to me... 😅

@hqhq
Copy link
Contributor

hqhq commented Mar 6, 2020

@hqhq: [...] but I'm not sure why we are not using it in the first place [...]

Go modules became mature with Go 1.13 (released Sept '19), and this PR has been open since June 19. Together with the usual turnaround time for reviews here, that already seems like more than enough explanation to me... 😅

I see, thanks for explain. BTW does this mean we are not able to build runc without go 1.13 or newer?

@odinuge
Copy link
Contributor Author

odinuge commented Mar 6, 2020

I see, thanks for explain. BTW does this mean we are not able to build runc without go 1.13 or newer?

We still keep the vendor/ directory with all the dependencies, so it will still work the same for pre go 1.13.

@h-vetinari
Copy link

Another conflict has arisen, unfortunately (due to merging #2222).

This removes vndr, and swiches to native Go Modules instead. All modules
are kept on the old version.

Keeps the vendor/ dir, so everything is backwards compatible.

Signed-off-by: Odin Ugedal <odin@ugedal.com>
Signed-off-by: Odin Ugedal <odin@ugedal.com>
Signed-off-by: Odin Ugedal <odin@ugedal.com>
Signed-off-by: Odin Ugedal <odin@ugedal.com>
Signed-off-by: Odin Ugedal <odin@ugedal.com>
Signed-off-by: Odin Ugedal <odin@ugedal.com>
Signed-off-by: Odin Ugedal <odin@ugedal.com>
@odinuge
Copy link
Contributor Author

odinuge commented Mar 7, 2020

Another conflict has arisen, unfortunately (due to merging #2222).

Ahh. Updated the PR now.

Since #2222 started building with go 1.14, and require it to pass, I switched back to running the vendoring in 1.14. If we vendor with go 1.13, go build will fail on go 1.14, making the tests fil.

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 7, 2020

LGTM

Approved with PullApprove

@AkihiroSuda
Copy link
Member

ping @opencontainers/runc-maintainers PTAL

@AkihiroSuda
Copy link
Member

I'll merge this on Wednesday unless there are any objections

@cyphar
Copy link
Member

cyphar commented Mar 16, 2020

LGTM. I do have some hesitation about some of the administrative aspects of Go modules (I've had issues with updating individual dependencies before, as well as problems with go mod vendor stripping out some source files that Debian needed) but we should still switch.

Approved with PullApprove

@cyphar cyphar closed this in a15d2c3 Mar 16, 2020
@cyphar cyphar merged commit a15d2c3 into opencontainers:master Mar 16, 2020
AkihiroSuda added a commit to AkihiroSuda/runc that referenced this pull request Mar 28, 2020
Background: opencontainers#2073 (comment)

> switched back to running the vendoring in 1.14. If we vendor with go
> 1.13, go build will fail on go 1.14, making the tests fil.

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda AkihiroSuda mentioned this pull request Mar 28, 2020
AkihiroSuda added a commit to AkihiroSuda/runc that referenced this pull request Mar 28, 2020
Background: opencontainers#2073 (comment)

> switched back to running the vendoring in 1.14. If we vendor with go
> 1.13, go build will fail on go 1.14, making the tests fil.

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
AkihiroSuda added a commit to AkihiroSuda/runc that referenced this pull request Mar 30, 2020
Background: opencontainers#2073 (comment)

> switched back to running the vendoring in 1.14. If we vendor with go
> 1.13, go build will fail on go 1.14, making the tests fil.

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
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.

9 participants