Another backwards compatibility break propagating through projects: rename logrus_syslog package to syslog - PROPOSAL

[cstockton]

Another pull request .. #108 about nomenclature.. and #587 renames logrus_syslog .. breaking all packages that use it.

# github.com/osrg/gobgp/gobgpd
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:30: syslog redeclared as imported package name
	previous declaration at /ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:21
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:53: undefined: "github.com/sirupsen/logrus/hooks/syslog".Priority
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:56: undefined: "github.com/sirupsen/logrus/hooks/syslog".LOG_KERN
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:58: undefined: "github.com/sirupsen/logrus/hooks/syslog".LOG_USER
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:60: undefined: "github.com/sirupsen/logrus/hooks/syslog".LOG_MAIL
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:62: undefined: "github.com/sirupsen/logrus/hooks/syslog".LOG_DAEMON
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:64: undefined: "github.com/sirupsen/logrus/hooks/syslog".LOG_AUTH
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:66: undefined: "github.com/sirupsen/logrus/hooks/syslog".LOG_SYSLOG
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:68: undefined: "github.com/sirupsen/logrus/hooks/syslog".LOG_LPR
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:70: undefined: "github.com/sirupsen/logrus/hooks/syslog".LOG_NEWS
/ws/wrk/src/github.com/osrg/gobgp/gobgpd/util.go:70: too many errors

I really am surprised that after the last fiasco with renaming this repo that I am dealing with yet another rename today. This time breaking all dependencies using the "syslog" package in exchange for the following value- justified by @dmathieu:

Package names shouldn't be using underscores. There is also nothing
wrong with naming this package syslog. People can include it with any
name they wish.

Yes- people can include it with any name they wish. So the people who are bothered by the name can use "logrus". I have no idea how you can justify breaking dependencies to remove an underscore....?

Anyways- this leads me into a proposal for @sirupsen, that is to FREEZE this API for master and please stop breaking projects that depend on this library. It is wildly popular, with that comes the responsibility to be stable. Incompatible API changes should be weighed very heavily and well thought out. "Just vendor" isn't an option, the ship has sailed and logrus predates some of the best practices that have emerged in the Go community. I believe it needs the stability to the standard libary.

A new import path and version could be made for a "cleaned" up version- I suggested some time ago in #451 (comment) for a logrus organization for this purpose, or something like gopkg.in. I think it would be beneficial to all library consumers though to find some form of resolution for stability in logrus.

[cstockton]

Oh- and of course I propose immediately reverting #587 before it's propagated into peoples workspaces as they update and really cause a mess. These changes can take a while to emerge as we saw with the last rename so the earlier you revert the less impact in the long run.

[sirupsen]

I think this is a legit concern, @dmathieu what was the motivation for the change?

[seiffert]

Hi @cstockton, thank you for taking the time to write this up.

As one of the maintainers of logrus and the one reviewing the change you refer to, I'd like to add my thoughts on this. Before getting into details, I'd like to repeat that I'm thankful for your input but would also ask you for a constructive discussion. I understand that you're angry about having to change import paths again though.

In general, we should all stick to the convention that binaries vendor their dependencies (and/or contain requirement files that are managed by a go package manager like dep). In my experience, logrus isn't the only library that changes its exported types and packages. Nevertheless, I understand that vendoring imposes other challenges.
Libraries on the other hand should (this is only my opinion, not necessarily an agreed-upon convention) not depend on a specific logging library and keep dependencies to third-party libraries as limited as possible.

In order to keep change possible at all, I'm currently 👍 for your suggestion to freeze the current API in master and start working in branches that then are referred to using gopkg.in. I will definitely take some time to discuss this with @dmathieu and @sirupsen in the next few days.

One thing that I don't understand is how you ended up in the situation of your error message though. Could you please elaborate on that?
I looked at the project you're referring to and found a "Gopkg.toml" that contains a constraint to logrus in version 1.0.2. Did you run dep ensure before or do you rely on the logrus version from your $GOPATH? Did you stumble upon this before the constraint was updated to point to the new logrus version?

Regarding your question @sirupsen: I'm pretty sure that @dmathieu wanted to get rid of the old issue #108 and followed the suggestion in there.

[cstockton]

Before getting into details, I'd like to repeat that I'm thankful for your input but would also ask you for a constructive discussion. I understand that you're angry about having to change import paths again though.

Not sure what about my post isn't constructive or what implies I'm angry. As stated I'm surprised to be running into more logrus issues, nothing more or less.

In general, we should all stick to the convention that binaries vendor their dependencies (and/or contain requirement files that are managed by a go package manager like dep). In my experience, logrus isn't the only library that changes its exported types and packages. Nevertheless, I understand that vendoring imposes other challenges.
Libraries on the other hand should (this is only my opinion, not necessarily an agreed-upon convention) not depend on a specific logging library and keep dependencies to third-party libraries as limited as possible.

I understand the position and agree that they should, simple fact is that they don't. Many applications, libraries, tools exist that were written long before dep, and predate vendoring as well. I could go on but I don't think the importance of backwards compatibility really needs justified in the Go ecosystem. I'm not sure about other libraries changing exported identifiers, I've never experienced this myself but would be willing to read through comments from an issue for an exported identifier that changed that has a dependency graph as massive as what logrus has.

One thing that I don't understand is how you ended up in the situation of your error message though. Could you please elaborate on that?
I looked at the project you're referring to and found a "Gopkg.toml" that contains a constraint to logrus in version 1.0.2. Did you run dep ensure before or do you rely on the logrus version from your $GOPATH? Did you stumble upon this before the constraint was updated to point to the new logrus version?

They define a Gopkg.toml, recently yes. But my docker containers that existed long before dep predate this, as well as many projects I've made in the past. I haven't even adopted dep yet and will wait as long as possible to do so, my first experience with dep was having it rm -rf my vendor folder which at least led to golang/dep#552. Even if I loved dep (I don't) and was excited for any opportunity to upgrade things to it as time goes on this becomes an impossible task. It's why backwards compatibility (go1promise) and such exist for highly depended upon libraries (like logrus), so developers don't have to randomly stop all work to upgrade a stable and completed project, subjecting it to test cycles, ci/cd, code review, all the noise along with it.

[cstockton]

Would the logrus team accept a pull request for this or will the change remain?

[dmathieu]

I don't think reverting the package name change is a good idea. We'll get other people saying they're relying on the latest version and it breaks again.

The true issue here is that we're unable to do anything on logrus except basic maintenance due to the impossibility of doing any breaking changes. This is the true thing we need to fix.

[cstockton]

@dmathieu I very strongly disagree.

I don't think reverting the package name change is a good idea. We'll get other people saying they're relying on the latest version and it breaks again.

For every person that complains it was broken and fixed again, there will be much more that report it was broken months from now. If you leave it broken you make sure every single person who has it in their dependency tree is affected. If you revert it the people who have been affected by the change are limited to those who upgraded earliest. We have already seen how these sort of issues take time to cascade through your packages dependency graph, the pain for the users is multiplied by the fact that the longer it takes for them to be impacted the further away the dependency is from their project and more difficult it can be to resolve.

The true issue here is that we're unable to do anything on logrus except basic maintenance due to the impossibility of doing any breaking changes. This is the true thing we need to fix.

The main issue is making backwards incompatible changes on a library which has a massive dependency graph. There are options that could be exercised here that don't involve breaking every users project for nomenclature purity. The value of this change measured against the expense of breaking every single dependency seems impossible to justify from me personally. Hopefully the fact this subpackage is less used will cause less trouble for users down the road.

While I strongly disagree with the Logrus maintainers that it is okay to simply break dependencies without strong justification, I accept it and thank you for at least hearing me out.

[dmathieu]

Let me rephrase. I, and the other maintainers of logrus ( @seiffert and @sirupsen ) are entirely aware that breaking backward compatibility is entirely shitty.

At this point though, properly working on improving the project will require a version 2.0 which will bring backward-incompatible changes. But we're stuck with it, as we can't bring them to master (they'd be pulled by everybody), and we can't really create a 2-0 branch, as no PRs would be based against it.

Basically, we're at a state where we either bring backward-incompatible changes, or the project slowly dies by not being updatable.
I' slowly starting to feel we should go with the second solution, and possibly start a new repository implementing the hooks API. But giving up like this really sucks.

[cstockton]

@dmathieu If you understand it's undesirable and I've yet to see justification for the change I don't understand why you don't want to revert it? I don't agree that you're options are limited here or that there is the slightest hint of contention while quantifying them. This is what I would do:

1. Revert this change immediately because the longer it exists the more people that will be affected, I've already explained the reasoning above so I won't reiterate.
2. Adopt the Go1 compatibility promise which means only breaking logrus API if ABSOLUTELY necessary. If someone complains the name of a package or publicly exported identifier is not idiomatic the issue is assigned a 2.0 tag. If someone raises a troublesome security concern that can only be mitigated via a API change- then the code is changed.
3. Use the logrus organization offered to you to resolve 2.0 tags. This would give the opportunity for sub repos containing plugins / log adapters to be maintained by separate individuals. You may give a completely fresh start for logrus where you can define expectations around backwards compatibility.

All users with the flexibility to immediately change logrus to a newer version to enjoy the improvements can do so early. Without forcing another year of pain as backwards incompatible changes cascade through the dependency graph of projects for users who are not positioned to upgrade immediately. I think Aliases show the importance of API compatibility and how when you have such a large dependency graph at that scale and velocity breaking changes are no longer an option. Logrus is at a much larger disadvantage then Google was even, since in logrus you replace velocity with disparity and remove the ability to update every dependency of a packages change.

My last two cents for @sirupsen I don't want to beat a dead horse or anything just really want to avoid what I see as a pretty awesome project come under anymore heat like the rename. Not when there are other perfectly reasonable options.

-Chris

[sirupsen]

Use the logrus organization offered to you to resolve 2.0 tags.

How will moving logrus to another organization not be another case of casegate?

[cstockton]

@sirupsen To clarify by "use the logrus org" I mean specifically to use it for future development while freezing the API for this repository. I don't know what options github has around keeping issues, stars and so on so I don't have an opinion about the semantics of the move. Only detail I think is important is that the repositories are separate and the current repository remains in tact.

Under this clarification we can see that changing cases gave people one option, stop what they are doing and make changes to their code. While creating a repository in a new organization for a v2.0 logrus and keeping the existing logrus at sirupsen/logrus with stricter compatibility solves all the major pain points:

1. People can upgrade to the new logrus when they choose to.
2. The new repo could be used to vet out the design by logrus users who have the flexibility (and desire) to test the new features or changes, allowing those who don't have the luxury for one reason or another to wait until the API is solidified.
3. Both import paths can live side by side allowing dependencies in your gopath to update at their own paces.
4. You may defines stricter expectations up front in the new logrus repo- such as you assume users will be using dep. This allows you to make tiny incompatible changes and experiment without frustrating users.