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 go module support #307

Merged
merged 1 commit into from
Nov 8, 2019
Merged

Add go module support #307

merged 1 commit into from
Nov 8, 2019

Conversation

saschagrunert
Copy link
Contributor

This PR adds a go.mod and go.sum to the project which contains the latest dependencies. It would be awesome if we could tag a release according to semver after this PR got merged, WDYT? 😇

@lucab
Copy link
Contributor

lucab commented Jun 13, 2019

We touched this topic in the past with @arithx, and we are a bit cautious.

Relevant sharp edges:

  • there is no overall go-systemd package, just individual sub-packages. Versioning is shared, though.
  • this repository had tagged releases since the beginning, but gomod doesn't understand the format. So the starting version likely has to be v20.0.0 (or whatever the next tag is).
  • the API re-exports types from other libraries (e.g. godbus). As such, I expect major-version bumps to be the norm (as it is currently done).
  • related to the above, I'd like not to have to move directories around for each breaking change (i.e. /vX -> /vY). I think this can/should be done via the top-level module definition.
  • gomod support in libraries consumed here still needs some adjustments, see go.mod doesn't follow requirements godbus/dbus#125

@Xuanwo
Copy link
Contributor

Xuanwo commented Jun 20, 2019

Maybe we can switch to go mod in next release?

Use https://github.com/coreos/go-systemd/v20 as module name just like https://github.com/google/go-github.

@lucab
Copy link
Contributor

lucab commented Jun 20, 2019

I'd like to do a couple of first-hand experiments before that, in particular wrt. subpackages and module-version, so likely not targeting this at the next release.
And yes, puttting the version label in the module directive was my approach in the above list.

@lucab
Copy link
Contributor

lucab commented Jun 20, 2019

Also relevant here: go-systemd consumes coreos/pkg which is a mix bag of messy things and not very curated. But, we strictly only need coreos/pkg/dlopen, which is quite stable and easier to version.

I'll check and arrange to get the latter moved to coreos/godlopen next, with its own semver and go module.

@lucab
Copy link
Contributor

lucab commented Jun 20, 2019

Additionally, it is my current understanding that we have to rewrite all our internal imports to be in the form github.com/coreos/go-systemd/vX/sub_package and keep the vX portion in sync with the module declaration.

@lucab lucab mentioned this pull request Jul 9, 2019
@Xuanwo
Copy link
Contributor

Xuanwo commented Jul 9, 2019

Yes, we need to update all files while upgrading major version.

Golang may add go mod upgrade to help reduce the work, but not accepted for now.

@saschagrunert
Copy link
Contributor Author

Hey, any idea how to move forward here? The latest version of godbus/dbus is now using a v5 import suffix which makes it hard to use go-systemd with projects using go modules.

@lucab
Copy link
Contributor

lucab commented Nov 1, 2019

@saschagrunert thanks for pinging back. I think we can more or less stick to the plan highlighted so far.
In particular, I think we can proceed as follows:

  • copy dlopen under an internal/ hierarchy here and adjust imports
  • release v21 with that, which would be the last version using dep
  • swap manifests dep->gomod, and introduce v21 in all imports
  • bump everything to v22 and release a v22.0.0 tag

I don't have any of this on my current radar as I'm unfortunately already fully-booked on other topics, but I'll gladly review any PR and try to quickly cut releases when ready. If you have a pressing timeline on this, feel free to start PRing.

@ulidtko
Copy link

ulidtko commented Nov 1, 2019

What a coincidence; I'm currently struggling with exactly this issue, too.

I don't feel proficient enough in Go to roll a PR, so the only thing left for me is to wait. Sigh...

@saschagrunert saschagrunert force-pushed the go-modules branch 2 times, most recently from 92eac10 to ee1f571 Compare November 4, 2019 10:50
go.sum Outdated Show resolved Hide resolved
@saschagrunert
Copy link
Contributor Author

saschagrunert commented Nov 4, 2019

Would it be fine if we drop the build for go 1.10/11? Because the v5 imports won't work with that.

@saschagrunert saschagrunert force-pushed the go-modules branch 2 times, most recently from 282bb4a to a219758 Compare November 4, 2019 11:01
@saschagrunert
Copy link
Contributor Author

PTAL

.travis.yml Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Signed-off-by: Sascha Grunert <[email protected]>
@chrboe
Copy link

chrboe commented Nov 8, 2019

Is there some kind of workaround for people trying to use this package in a module environment, or will we just have to wait around for this to get merged?

@saschagrunert
Copy link
Contributor Author

Is there some kind of workaround for people trying to use this package in a module environment, or will we just have to wait around for this to get merged?

I right now stick to an older version.

@lucab do you think we can move forward here?

@lucab
Copy link
Contributor

lucab commented Nov 8, 2019

Ah, I didn't realize this was re-pushed after my last review. LGTM now, I'll merge.

@lucab lucab merged commit 5a0db84 into coreos:master Nov 8, 2019
@saschagrunert saschagrunert deleted the go-modules branch November 8, 2019 16:59
@chrboe
Copy link

chrboe commented Nov 8, 2019

Wow, that was quick! Thanks

@saschagrunert saschagrunert mentioned this pull request Nov 8, 2019
@lucab
Copy link
Contributor

lucab commented Nov 11, 2019

Tagged: https://github.com/coreos/go-systemd/releases/tag/v22.0.0

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.

5 participants