Case change breaks builds?

[lonelycode]

Not sure what's started to happen, but overnight our builds have started to break:

can't load package: package github.com/TykTechnologies/tyk: case-insensitive import collision: "github.com/Sirupsen/logrus" and "github.com/sirupsen/logrus"

I wonder if it's the case change from S to s in the github username of the account. Even if we change the import path to use the lowercase version, dependencies that use the uppercase version that are not under our control seem to need updating too.

Vendoring the lib doesn;t seem to work either, as the log.WithFields seems to self referential, but maybe I was too hasty in dismissing vendoring:

./api.go:124: cannot use "github.com/TykTechnologies/tyk/vendor/github.com/Sirupsen/logrus".Fields literal (type "github.com/TykTechnologies/tyk/vendor/github.com/Sirupsen/logrus".Fields) as type "github.com/Sirupsen/logrus".Fields in argument to log.WithFields

Any tips appreciated.

[lonelycode]

Disregard the vendoring bit, I think that's our centralised logger screwing up.

Edit: And trying to fix it, by vendoring within the common logger, causes the same fields issue. This change really has borked our builds.

[cstockton]

I can't get gobgpd to build anymore either, but I'm having trouble tracking down why.

$ go get -v -u github.com/osrg/gobgp/gobgpd
github.com/osrg/gobgp (download)
package github.com/osrg/gobgp/gobgpd: case-insensitive import collision: "github.com/Sirupsen/logrus" and "github.com/sirupsen/logrus"

The error doesn't say where the culprit file is, a grep through ALL my dependencies show that the only package that imports a lower case version in all my dependencies is Sirupsen/logrus

$ grep -r "github.com/sirupsen/logrus" ~/dev/local/go-workspaces/dependencies/
src/github.com/Sirupsen/logrus/alt_exit_test.go:    "github.com/sirupsen/logrus"
... snip ....
src/github.com/Sirupsen/logrus/README.md:  logrus_syslog "github.com/sirupsen/logrus/hooks/syslog"
src/github.com/Sirupsen/logrus/README.md:| [Syslog](https://github.com/sirupsen/logrus/blob/master/hooks/syslog/syslog.go) | Send errors to remote syslog server. Uses standard library `log/syslog` behind the scenes. |
src/github.com/Sirupsen/logrus/README.md:  log "github.com/sirupsen/logrus"

So looking at this, you have Sirupsen importing the lower case version..? I think you need to roll this back and come up with a better name change strategy because anything I try to build that imports your package breaks. I tried to cp -a Sirupsen to sirupsen .. apparently TIL[1], so I can't have them side by side.. I don't know how to fix this other than adding another go path with a lower case version just for your library. Anyways, everyone is going to run into this at some point I think it needs rolled back.

@sirupsen is there a way for me to fix this? I can't change the name to lowercase on my FS, or change it to Upper.. both break.

[1] http://apple.stackexchange.com/questions/22297/is-bash-in-osx-case-insensitive

[cstockton]

@lonelycode Did you happen to find a fix for this? Everything I try results in breakage, least on OSX because I can't have both on the same file system. Changing my GOPATH still broke it. For now I am just checking out a older version so I can get up and running locally, but all my docker-compose / automation needs a work around if you happen to find one, thanks.

[lonelycode]

We can't afford to have something like this happen anymore, so we've vendored everything so we can control the namespace.

Since we use multiple logrus plugins (both from @sirupsen and from external contributors), basically:

• Internal references use the lowercased version
• external deps that have not been updated use the uppercase version

The two cannot live together at all, vendoring will not work, because external plugins (and internal ones) require the lowercased version as a type and you will either get a FGield type mismatch or you will get a case insensitivity issue.

So the only solution was to take it all in house :-/

[hilljgo]

Take a look at pull request 384 that got merged yesterday for reference

[mbetz08]

This also broke us. Can you revert PR 384 @sirupsen?

[lonelycode]

Sorry for the long reply...

Conventions are great - I'm a big believer in solid conventions and following them - but following conventions should be done before creating a dependency tree.

Changes like this have a significant impact downstream, especially if those projects use third-party libs/plugins that havent updated their import paths so the only solutions are to:

1. Wait for these third party Devs to update their code (or accept your PR - which still leaves you in the lurch)
2. Internalise everything
3. Work off of pinned revisions (a very unpalatable option)

We use 3 or 4 hooks, some provided by this repo and others external. And they all disagree as to what to reference, this change broke the build for about 5 of our projects and about 3 dependent internal libs.

Personally I prefer to not vendor because it means that stable, non-breaking updates make it into your build organically, and breaking changes are usually documented by library maintainers well enough to make for a painless transition - non-breaking backwards compatibility is pretty core to Go AFAIK, and it's a pattern to be followed, because it encourages responsibility on bahalf of the library maintainer.

But because logrus is a pluggable lib, it has an ecosystem that is dependent on its stability, and like all ecosystems those extensions only update at the behest of their maintainers - which is never a given in OSS.

That means changing the core dependency so it breaks the ecosystem should be done with great care, because now after this PR, the logrus ecosystem is broken and unusable until all hook developers, and formatters, and plugin builders update their code.

We've mitigated this upstream risk by forking and modifying all dependent projects -including logrus - because we can't wait for the ecosystem to self-correct, that was effort we could have expended elsewhere.

(Yes these should all be pull requests to the existing maintainers, and they will be - but to get to production waiting for a PR in an automated environment is not an option)

This is a great library, so please keep making it better and extending it, but please consider introducing major breaking changes more carefully.

[sirupsen]

Yes, this was due to https://github.com/sirupsen/logrus/pull/384.

please consider introducing major breaking changes more carefully

We did. These things are not taken lightly. Logrus favours stability at this point over features and re-design. On the other side of the coin, you have a large body of people who had issues with the previous casing. I believe that this is the most sustainable path forward.

I apologize for the pain this has caused everyone in this thread. I understand that the last thing you want to have break your builds, is your logging library. I want to make sure you understand that Logrus favour stability and backwards compatibility over all else. This was not an easy decision, but we want Logrus to adhere to standards. I run production too, and I understand for 99% of Logrus users they use the standard API, community extensions and don't care much for a logging library failing their build.

Logrus is a foundational piece of infrastructure, and it should, and will, change minimally.

Logrus is in need of a healthy maintainer group, and if you think you can help us make better decisions, I'd love to have you join us and help @lonelycode.

[lonelycode]

Thanks for the response @siripsen, I'm sure the decision wasn't taken lightly. I's a great library (we love it), it was just very frustrating watching our builds fail overnight and then not finding a clean update path.

Happy to get involved - will start sending out those PRs :-)

[hryx]

For any Glide users affected by this change, the GitHub name change also poses a problem. See a workaround here: https://github.com/sirupsen/logrus/pull/384#issuecomment-264035988

The solution we went with for now was locking to an old Logrus version and making the repo URL explicit.

[cstockton]

This still hasn't been rolled back, @sirupsen are you keeping this change? Every single persons build who uses your library is broken right now.. over a capital letter. If you truly felt it was imperative to have a all lowercase package name and the project is your #1 priority- why not move it into it's own organization with a new name all together to give people a chance to migrate.. github.com/logrus/logrus or something.

I think this should be reverted and reevaluated, there is not even a fix for this. I would need to hunt down every single dependency of every library.. and have them update in a single transaction across multiple organizations and repos .. all to change the casing of a letter to .. what? Keep your name inside the url? Seems petty.. just my opinion, it's your repo, name and code.

[sirupsen]

Are we sure that a revert is not going to cause even more pain at this point?

@stevvooe what do you think?

[sirupsen]

Given the backlash here I'd like to say I'm seriously considering reverting. I'm very surprised by the amount of people who don't have Logrus vendored, or in other words, how many people this is breaking for.

[sirupsen]

Is this really as simple as reverting that PR? Doesn't it mean I need to rename myself back to @Sirupsen as well?

[cstockton]

@sirupsen Reverting the PR will not cause more pain, people who solved this problem by vendoring will still be vendored. People who can't vendor because they don't control the build system, repos, etc will be fixed. You don't need to revert your name since both Sirupsen and siruspen resolve properly, so only the pull request needs reverted.

[sirupsen]

On it.

[sirupsen]

Fixed in master. Let me know if this works for you. Thanks for providing adequate context.

[sirupsen]

I've added a public comment about this to the README should anyone come across this.

I apologize for this. I underestimated the impact this had. This was not acceptable, and not a justified thing to break backwards incompatibility for. If we wish to provide a lower-case name-change, we'll do so under a different name (e.g. under a logrus organization).

[sirupsen]

I'm keeping this issue open as some people may browse here looking for an open issue.

[cstockton]

@sirupsen Classy, I respect your grace with this. This resolved the issue for all the automated builds that use go get on repos that don't vendor. On the local repo copy I still have, continued to work fine after go get -u. The only users that may have been affected would be anyone who was able to update their code since this morning, I imagine those users can quickly pivot back just the same. Thanks for fixing it for the rest of us.

[stevvooe]

We didn't have any build failures, since we use vendoring, but I was able to reproduce this earlier today with gobgp.

On my local box (OS X), where I use go get extensively, I updated by moving github.com/Sirupsen to githhub.com/sirupsen. You can try to clear out $GOPATH/pkg in case there are mixed case packages lingering. This works fine for shallow imports.

With larger projects, more is required. This is the build error I received with gobgp:

$ go build github.com/osrg/gobgp/gobgpd
can't load package: package github.com/osrg/gobgp/gobgpd: case-insensitive import collision: "github.com/Sirupsen/logrus" and "github.com/sirupsen/logrus"

Nothing a little perl can't solve:

$ find ~/go/src/github.com/osrg/gobgp -type f -name '*.go' | xargs perl -i -pne 's|Sirupsen|sirupsen|'

After running that, the build works fine.

This is probably not a "production" solution, but neither is "go get".

[shusugmt]

@stevvooe https://github.com/osrg/gobgp/pull/1182 I believe now we don't have any problem on gobgp master w/o any modification because the package name has reverted a couple hours ago

[stevvooe]

@s2ugimot Thanks! I saw the PR, since I was going to upstream it. Too bad this was rolled back.

Does gobgp deploy production code built with go get?

[shusugmt]

@stevvooe

Does gobgp deploy production code built with go get?

Yes. But I found this problem when I was playing around with it on lab, not for building binary for production deploy. On production environment we build binaries within the docker container using go get and preserve it so that we can safely reproduce the same binary later on. Dependent package name change like this is one of the exact example which we have expected to happen. In this case it was reverted anyways...

[stevvooe]

preserve it so that we can safely reproduce the same binary later on

At least you can audit later if malicious code gets pulled down.

I'd highly recommend exploring vendoring for repeatable builds. It does seem clunky but is rock solid. You can still have a "side-build" that deletes the vendor directory to keep track of upstreams are breaking.

[shusugmt]

@stevvooe Thanks, I'll look into vendoring. And I think I also need to talk to the developers: https://github.com/osrg/gobgp/issues/963

[cstockton]

@stevvooe there was plenty of ways to resolve it, but only one way that resolved itself. I'm not sure what your point is about "go get" and production, or "gobgp" production code being built with "go get". This isn't a production issue, no one here was claiming they had systems down.

The complaint was a needless interruption in everyones workflow. Maybe you don't, but I along with many other developers manage their many (or one) GOPATHS for local development using go get. I don't vendor every single command, binary, project, etc that I use day-to-day. Many of those projects don't vendor their own libraries.

With how fast the Go ecosystem moves and interesting new libraries are popping up I don't see how someone could ever discourage the use of go get, then to justify breaking many peoples workflows over a capital letter on their file system system.. just seems unusual to me.

@s2ugimot I think vendoring may be a good idea given the scale of GoBGP and the fact the binary commands (gobgp gobgpd) are widely used ad-hoc with go-get.

[tobbbles]

I side with @sirupsen's suggestion here https://github.com/sirupsen/logrus/pull/384#issuecomment-264333311, in were he suggests this to be continued in logrus/logrus.

This ensures support for those vendoring, or go get'ing, either upper or lower cases.

However - I feel a repo change shouldn't be purely on a naming and conventional basis, but should have a larger driving factor.

[kcmerrill]

vendor/github.com/Sirupsen/logrus/examples/hook/hook.go:12: cannot use airbrake.NewHook(123, "xyz", "development") (type *airbrake.airbrakeHook) as type "github.com/kcmerrill/crush/vendor/github.com/Sirupsen/logrus".Hook in argument to log.Hooks.Add: *airbrake.airbrakeHook does not implement "github.com/kcmerrill/crush/vendor/github.com/Sirupsen/logrus".Hook (wrong type for Fire method) have Fire(*"github.com/Sirupsen/logrus".Entry) error want Fire(*"github.com/kcmerrill/crush/vendor/github.com/Sirupsen/logrus".Entry) error

^^^ just for anybody who is googling(this is the error I was getting). Took me a while to find this thread(didn't realize readme was the problem I was experiencing).

@sirupsen thanks for all your work with logrus. ❤️ it.

[sirupsen]

Reverted, so I'm going to close this.

[gravis]

I'm getting very confused with this. It seems the change is back for good (https://github.com/bshuster-repo/logrus-logstash-hook/issues/32#issuecomment-303195028), could you confirm please?

[sirupsen]

@gravis upper case works fine, but if you are using the examples it may force you to use lower-case—or if you are mixing dependencies that use lower-case and upper-case. We need to converge towards lower-case, because this is a mess no matter how you slice it. I'm really regretful we are in this situation, but I failed to consider the cascading impacts renaming would have.

[gravis]

Ok thanks. I have a bunch of hooks to update then, I hope users will understand. I think I would have fallen in the same trap as you :(

[sirupsen]

Thank you for your understanding @gravis 😢 It's a regretful situation, but I hope as a result people update their dependencies and get the best out of it.

[cstockton]

I think the pain point for this bug is just how subtle it is.. even more so on FS with case folding. It's unfortunate you didn't decide to move to a new repo, everyone could have used both versions side by side and upgraded painlessly. With aliases coming you could eventually nudge people further to the new repo with aliases. Oh well.

[sirupsen]

@cstockton I agree with you, and I am aware of that mistake. Did not realize the consequences when we did the initial try.

[cstockton]

@sirupsen My point was just that if you agreed with my comment here https://github.com/sirupsen/logrus/issues/451#issuecomment-264319311 then I'm confused why you would start pushing the breaking change months later. But it's your project, thanks for being polite regardless.

[crazyobject]

@sirupsen - I changed my gomfile and lowercased it, but still I am getting following error in both cases ( Capital and small ). can't load package: package github.com/sirupsen/logrus/examples/hook: case-insensitive import collision: "github.com/Sirupsen/logrus" and "github.com/sirupsen/logrus". What should I do now.

[sirupsen]

Why are you importing the example hook?

[crazyobject]

My gomfile have following line gom github.com/sirupsen/logrus

But while doing a build it shows above error, I have not written anything for examples/hook.

[gravis]

what do you think of this? https://github.com/meatballhat/negroni-logrus/issues/28#issuecomment-305216526

[sirupsen]

@gravis I'm concerned that if we start to use gopkg too many paths will collide, we already have two alternative paths.. this would give a third 😬 That would be the best thing long-term, absolutely—to give the freedom to iterate on the API.

[gravis]

Hmm, you're right. Ok, so let's move to sirupsen then. Thanks!

[aeneasr]

This is confusing, what's the right way to do this now? In the readme it says:

Seeing weird case-sensitive problems? See this issue. This change has been reverted. I apologize for causing this. I greatly underestimated the impact this would have. Logrus strives for stability and backwards compatibility and failed to provide that.

but it also says:

The organization's name was changed to lower-case--and this will not be changed back. If you are getting import conflicts due to case sensitivity, please use the lower-case import: github.com/sirupsen/logrus. - Source

[sdboyer]

just wanted to let folks who're monitoring this issue know that golang/dep will soon have some more sophisticated reporting around this problem: golang/dep#1079. we can't actually solve it in dep - doing so requires rewriting import statements, which we strictly do not do - but we can guide the user to where the problem is, and to know which projects need issues filed against them to change from S -> s.

[stevvooe]

@sdboyer Any chance at getting this fixed in the upstream Go compiler? This case is particularly insidious in that they can't actually exist next to each other.

[sdboyer]

@stevvooe depends on what you mean by "fixed"? all the compiler can do is tell you that it's not allowed to have import paths that vary only by case - which it actually does: https://github.com/golang/dep/issues/433#issuecomment-317146614 is that what you meant?

i know i was certainly happy when a contributor pointed that out, as i'd assumed the compiler just didn't care, which meant that dep would be disallowing a case that the compiler allows. that would've caused a ruckus.

[stevvooe]

@sdboyer I think the problem here is that there is case insensitivity until there is not. It's been awhile since I've investigated, but, if I remember correctly, the type name ends up being case sensitive, causing the collision.

I don't think dep can do anything here. The compiler would probably have to flatten case for the package paths, but I am unclear on the specifics.

[sdboyer]

there is case insensitivity until there is not.

i'm not sure exactly what you mean with this, but i'm guessing it amounts to "it isn't dealt with in a formal way, so it becomes a problem incidentally later." but...

the type name ends up being case sensitive, causing the collision.

it's not type names that blow up incidentally later; there's a formal, first-class check that disallows case variance in import path names. this is referenced in my earlier link: https://github.com/golang/dep/issues/433#issuecomment-317146614. quoting from there:

go build errors out when both cases are present on a case-sensitive filesystem (ext4): XXXXX: case-insensitive import collision: "github.com/sixgill/service/vendor/github.com/Sirupsen/logrus" and "github.com/sixgill/service/vendor/github.com/sirupsen/logrus"

this is the first-class check in the compiler that produces the error.

I don't think dep can do anything here.

yes, as i initially said, dep cannot fix the problem, because we cannot actually rewrite import paths. but dep can guide the user to other versions of a dependency that don't have a casing conflict (usually the desirable behavior, though sometimes surprising). golang/dep#1079 explains a bit more about the mechanics of this, but it's gonna be a bit difficult to grok in its entirety, as this is adding a new class of check to the domain-specific SAT solver at the core of dep.

[stevvooe]

@sdboyer What I mean is that case-sensitivity is not enforced until the user can longer deal with the problem. What is the point in allowing the symbol golang.docker.io/docker/vendor/github.com/Sirupsen/logrus.Errorf if introducing golang.docker.io/docker/vendor/github.com/sirupsen/logrus.Errorf just causes a compiler error? These should be case-folded much earlier in the process.