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

dbus: Fix API break from godbus #296

Merged
merged 2 commits into from
Oct 30, 2018
Merged

Conversation

purpleidea
Copy link
Contributor

Something in godbus seems to have broken API, thus breaking builds
including go get. This patches fixes those changes. I don't know any
background about this change, so if anyone has information, I'd sure
like to know! Thanks

@purpleidea
Copy link
Contributor Author

Here's the fix for #295

@lucab
Copy link
Contributor

lucab commented Oct 30, 2018

This is the same as #293 and #294.

@purpleidea
Copy link
Contributor Author

@lucab Yeah just saw. Well, everything seems to work with git master. Libraries like this shouldn't depend on old version or vendor. Make the updated patch will pass??

@purpleidea
Copy link
Contributor Author

Hm... So what's the plan to fix this??

@lucab
Copy link
Contributor

lucab commented Oct 30, 2018

Indeed, we don't vendor nor depend on old versions here, we keep the peace of whatever is the latest go-dbus release. We test with that and we communicate it to consumers via dep manifest. Usual dependency pinning+vendoring should be done by top-level consumers. @purpleidea where did you get a breakage?

Medium term, I'll be happy to update here and release as soon a new go-dbus release is out. I don't know what @jsouthworth plans are yet though.

@purpleidea
Copy link
Contributor Author

@lucab It broke the builds in https://github.com/purpleidea/mgmt/ but a much easier way to show the breakage is by running: go get github.com/coreos/go-systemd/ ...

Usual dependency pinning+vendoring should be done by top-level consumers.

For this type of library, there shouldn't be any vendoring, it should work with git master... The broken go get experience is the best proof of that.

Why aren't we merging this patch now?? Maybe I'm missing something?

@jsouthworth
Copy link

Given that this was unintentional, I'll tag a new dbus release now. It didn't occur to me that folks would be passing the functions around so there would be type conflicts, it should have, sorry about that. In other cases (direct calls) this would have been an invisible change since the default case is equivalent.

@jsouthworth
Copy link

@purpleidea
Copy link
Contributor Author

Okay, fixed patch updated with new version. Although I don't think we should have the toml file.

@purpleidea
Copy link
Contributor Author

purpleidea commented Oct 30, 2018

I've added a second patch to modernize travis. If you'd like them squished, I can do that, but I think it's better this way.

@purpleidea
Copy link
Contributor Author

(As an aside, I think the GH webui has a bug, because in my tree the travis patch comes first, although they are displaying backwards. Neat bug!)

@purpleidea
Copy link
Contributor Author

@lucab \o/ Tests passed, ready for merge =D

This removes the horribly old 1.5x and replaces it with golang 1.7 which
is apparently still used somewhere.
It also adds tests for the most recent version: 1.11.
Please upgrade or use older releases.
Something in godbus seems to have broken API, thus breaking builds
including `go get`.

As an aside, I don't think *this* project should include the Gopkg.toml
file. We should always follow git master automatically. Having said that
I've patched it anyways until we can discuss more.
@lucab
Copy link
Contributor

lucab commented Oct 30, 2018

@jsouthworth thanks!

@purpleidea thanks for the fixes. The two commits are fine, I just have a minor nit, after which we can merge. FWIW mgmt vendor directory is missing both coreos/go-systemd and godbus/dbus, which is the reason why it broke here and why it is likely to suddenly break again in the future.

@purpleidea
Copy link
Contributor Author

@purpleidea thanks for the fixes.

np

The two commits are fine, I just have a minor nit

What is it?

FWIW mgmt vendor directory is missing both coreos/go-systemd and godbus/dbus, which is the reason why it broke here and why it is likely to suddenly break again in the future.

I respectfully disagree. I believe that responsible libraries should always keep git master stable, and if there really is a reason to break API, then to ideally warn projects, and do it quickly. In any case, if I had vendored my deps, I wouldn't know what broke until way later down the road, and then finding and fixing the issue would be more expensive.

In the history of mgmt, we've only had builds break because of git master changes about four times. And all we're fixed right away, either by a small fix on our side, in the upstream, and in at least one case we caught a bug that a dep didn't catch in their CI.

I probably would have caught this break earlier except I've been AFK a bit lately.
The only dep we vendor is the weirdo deps for etcd grpc since those projects aren't responsible about git master and I can't replace that :(

Hopefully I can convince you to move to pure git master =D

@lucab
Copy link
Contributor

lucab commented Oct 30, 2018

Ack. The nit was on the Travis thing. I understand your point and the whole thing is a bit blurry in Go-land. LGTM, merging.

@lucab lucab merged commit f3b2571 into coreos:master Oct 30, 2018
@purpleidea
Copy link
Contributor Author

purpleidea commented Oct 30, 2018 via email

@peterfraedrich
Copy link

I'm still getting dep errors when importing coreos/go-systemd because there has been no new tag with this godbus/dbus fix yet. Can someone please cut a new tag (v18 i guess?)?

Fetching sources...

Solving failure: No versions of github.com/coreos/go-systemd met constraints:
	v17: Could not introduce github.com/coreos/go-systemd@v17, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v16: Could not introduce github.com/coreos/go-systemd@v16, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v15: Could not introduce github.com/coreos/go-systemd@v15, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v14: Could not introduce github.com/coreos/go-systemd@v14, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v13: Could not introduce github.com/coreos/go-systemd@v13, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v12: Could not introduce github.com/coreos/go-systemd@v12, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v11: Could not introduce github.com/coreos/go-systemd@v11, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v10: Could not introduce github.com/coreos/go-systemd@v10, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v9: Could not introduce github.com/coreos/go-systemd@v9, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v8: Could not introduce github.com/coreos/go-systemd@v8, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v7: Could not introduce github.com/coreos/go-systemd@v7, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v6: Could not introduce github.com/coreos/go-systemd@v6, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v5: Could not introduce github.com/coreos/go-systemd@v5, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v4: Could not introduce github.com/coreos/go-systemd@v4, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v3: Could not introduce github.com/coreos/go-systemd@v3, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v2: Could not introduce github.com/coreos/go-systemd@v2, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v1: Could not introduce github.com/coreos/go-systemd@v1, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	0: Could not introduce github.com/coreos/go-systemd@0, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	master: Could not introduce github.com/coreos/go-systemd@master, as its subpackage github.com/coreos/go-systemd does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)

@peterfraedrich
Copy link

opened #297 to track new release tag

@purpleidea
Copy link
Contributor Author

purpleidea commented Oct 30, 2018 via email

@peterfraedrich
Copy link

@purpleidea without explicit deprecation of dep support the fix shouldn't be don't use dep, it should to be cut a new release tag. if the repo maintainers want to deprecate their support for dep then it should be communicated in advance and at the very least with the last dep release being stable

@purpleidea
Copy link
Contributor Author

purpleidea commented Oct 30, 2018 via email

@lucab lucab added this to the v18 milestone Oct 31, 2018
@h8liu
Copy link

h8liu commented Oct 31, 2018

btw, same issue for github.com/coreos/container-linux-config-transpiler, which vendors go-systemd packages but not dbus packages.

@lucab
Copy link
Contributor

lucab commented Nov 1, 2018

@h8liu as far as I can tell, container-linux-config-transpiler should have no problems as it doesn't consume go-systemd/dbus. Travis seems to agree as the current master is green. If you are seeing problems building that, can you please report it at https://github.com/coreos/bugs? It may be something being off in your GOPATH, or a breakage in newer toolchain.

@h8liu
Copy link

h8liu commented Nov 2, 2018

@lucab , fyi, here is the build that is failing on gopkgs.io:
https://gopkgs.io/builds/20181102-060850-8e2dbf73

gopkgs.io tries to build every package in a git repo. the package is not actually imported by anything from the repo though, but it is vendored. it might be an bug/issue of the vendor manager that the repo is using..

thaJeztah added a commit to thaJeztah/runc that referenced this pull request Mar 28, 2019
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- coreos/pkg@v3...v4
  - no changes in vendored code
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/runc that referenced this pull request Mar 28, 2019
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- coreos/pkg@v3...v4
  - no changes in vendored code
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/runc that referenced this pull request Apr 12, 2019
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- coreos/pkg@v3...v4
  - no changes in vendored code
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/runc that referenced this pull request Apr 12, 2019
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- coreos/pkg@v3...v4
  - no changes in vendored code
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/runc that referenced this pull request Apr 12, 2019
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- coreos/pkg@v3...v4
  - no changes in vendored code
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/runc that referenced this pull request Apr 25, 2019
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- coreos/pkg@v3...v4
  - no changes in vendored code
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/runc that referenced this pull request Jul 14, 2019
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- coreos/pkg@v3...v4
  - no changes in vendored code
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/runc that referenced this pull request Aug 26, 2019
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/runc that referenced this pull request Sep 5, 2019
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
stefanberger pushed a commit to stefanberger/runc that referenced this pull request Dec 31, 2019
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
adrianreber pushed a commit to adrianreber/runc that referenced this pull request Feb 10, 2020
- https://github.com/coreos/go-systemd/compare/v14..v19
  - coreos/go-systemd#248 dbus: add SetPropertiesSubscriber method
  - coreos/go-systemd#251 activation: add support for listeners with names
  - coreos/go-systemd#296 dbus: Fix API break from godbus
- https://github.com/godbus/dbus/compare/v3..v5.0.1
  - godbus/dbus#89 introduce MakeVariantWithSignature

Signed-off-by: Sebastiaan van Stijn <[email protected]>
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