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.
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.
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
@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.
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:
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 :-/
Take a look at pull request 384 that got merged yesterday for reference
This also broke us. Can you revert PR 384 @sirupsen?
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:
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.
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.
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 :-)
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.
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.
Are we sure that a revert is not going to cause even more pain at this point?
@stevvooe what do you think?
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.
Is this really as simple as reverting that PR? Doesn't it mean I need to rename myself back to @Sirupsen as well?
@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.
On it.
Fixed in master. Let me know if this works for you. Thanks for providing adequate context.
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).
I'm keeping this issue open as some people may browse here looking for an open issue.
@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.
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".
@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
@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?
@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...
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.
@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
@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.
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.
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.
Reverted, so I'm going to close this.
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?
@gravis upper case works fine, but if you are using the examples it may force you to use lower-case鈥攐r 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.
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 :(
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.
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.
@cstockton I agree with you, and I am aware of that mistake. Did not realize the consequences when we did the initial try.
@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.
@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.
Why are you importing the example hook?
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.
what do you think of this? https://github.com/meatballhat/negroni-logrus/issues/28#issuecomment-305216526
@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鈥攖o give the freedom to iterate on the API.
Hmm, you're right. Ok, so let's move to sirupsen then.
Thanks!
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
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.
@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.
@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.
@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.
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.
@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.
@stevvooe ok, i'm a bit confused at this point, as it seems like you're trying to convince me to do something that we're already doing. maybe we're just talking past each other somehow. because:
What is the point in allowing the symbol
golang.docker.io/docker/vendor/github.com/Sirupsen/logrus.Errorfif introducinggolang.docker.io/docker/vendor/github.com/sirupsen/logrus.Errorfjust causes a compiler error?
yes. agreed. 馃挴. and disallowing that case is _exactly_ what we're doing, because it causes a compiler error. i've said or alluded to as much in every one of my last three comments, and the links i've already provided add more detail.
@sdboyer My question has been asking about a whether or not this should be a fix in the Go toolchain (see https://github.com/sirupsen/logrus/issues/451#issuecomment-326649038). There is no point in making the dep tool, go get and GOPATH support case-sensitive import paths when the compiler explicitly disallows it.
Packages with the same name but different casing need to be treated as the same package. While golang/dep#1079, it is unrealistic to have the solver look for solutions that will select a variant of github.com/Sdboyer/deptest based on what is imported. In reality, I want this to use whatever package is canonically folded to github.com/sdboyer/deptest. There is no use case for having multiple instances, even of they all agree on the same import casing.
My question has been asking about a whether or not this should be a fix in the Go toolchain
yes, i saw that, but that doesn't make sense. the go toolchain doesn't _do_ any kind of version selection right now. that's the problem we're fixing in dep. there is no conceivable way of "fixing" this in the go toolchain, apart from moving the responsibility for version selection into the go toolchain, which is the point of dep in the first place.
There is no point in making the dep tool, go get and GOPATH support case-sensitive import paths when the compiler explicitly disallows it.
i don't know why you think anyone is proposing this. i'm not. the PRs aren't. the PRs here REMOVE case sensitivity from (portions of) the import path, in a way that brings them into line with the compiler.
Packages with the same name but different casing need to be treated as the same package.
yes. agreed. and that's what the changes to dep in that PR are enforcing.
it is unrealistic
i'm not sure what this means. the PR literally works, so it's "realistic" in that sense. and it adds no new crazy logic in the solving algorithm - just one more dimension of satisfiability, wherein we verify that all projects active in the depgraph agree on a case variant for all project roots. now, that's not nothing - it's bumping the constant factor on an NP-hard search algorithm - but adding one more dimension doesn't just push us from "realistic" to "unrealistic."
In reality, I want this to use whatever package is canonically folded to
github.com/sdboyer/deptest. There is no use case for having multiple instances, even of they all agree on the same import casing.
so, wait - you're saying you want the system (the compiler, dep, etc.) to _enforce_ that import paths must be written in their case-folded form? if so, then OK, we do have a disagreement, at minimum because:
github.com/foo/bar) and "package" portions of import paths, at minimum because the former are governed by hosting platforms and the latter by code authors. doing that is not always possible without network access for go-get metadata, and that's a place the compiler can't go.but maybe i'm still not understanding, and that's not what you mean?
@sdboyer
yes, i saw that, but that doesn't make sense. the go toolchain doesn't do any kind of version selection right now. that's the problem we're fixing in dep. there is no conceivable way of "fixing" this in the go toolchain, apart from moving the responsibility for version selection into the go toolchain, which is the point of dep in the first place.
No one said we should fix this with versioning. The problem is that the compiler enforces that only a single casing of a package _import_ exists. I am not sure why you think there is no conceivable way of fixing this in Go. Do you have an analysis showing this? Go does import resolution within GOPATH and there may be a possible fix there that doesn't involve a versioning system.
__The issue right now is a project and all of its imports need to have the same casing for a package import__. If the compiler allowed mixed casing and only raised an error when the package is ambiguous (two casing versions are encountered at the same import priority), we could correct the problem by modifying the set of imports (for example, make sure that only github.com/sdboyer/deptest and not github.com/Sdboyer/deptest), rather than having all of the imported projects agree on a single import path.
Case-folding import paths would not have an effect existing Go code. No one expects github.com/sdboyer/deptest and github.com/Sdboyer/deptest to be different packages. Adding this as a constant factor in the SAT solver seems like a waste, when the package name could be canonicalized to github.com/sdboyer/deptest before adding the case variations to the solution space. When I say this is unrealistic, it means that it doesn't actually help with the problem; all packages have to agree on a single casing of import paths. __I am arguing that this shouldn't be a requirement in the first place and making this a requirement is what makes the problem untenable.__
Put simply, there may be a solution that exists by moving the enforcement of case variations to the package resolution stage, rather than based on encountered imports. It would then be the problem of dep or some other dependency system (GOPATH, vendor, etc.) to ensure that canonicalized import paths are unique, which is a solvable problem.
I am not sure why you think there is no conceivable way of fixing this in Go. Do you have an analysis showing this?
ahhh, i think i finally see what you're arguing for here. at least, the general shape of it.
my impossibility claim there was based on the assumption that you _were_ talking about doing this via version selection. now that i better understand what you're saying, though, sure - i suspect it's quite possible to do what you're suggesting. it's not clear to me whether it's the right path, though.
two casing versions are encountered at the same import priority
i'm not sure what "import priority" is, but "priority" suggests some concept of ordinality. while we can impose something like that on a DAG with a "shortest/longest path distance from root", it's not clear to me that that's actually a useful distinction for this domain in general, or for this problem in particular. hard to tell though, because:
we could correct the problem by modifying the set of imports (for example, make sure that only
github.com/sdboyer/deptestand notgithub.com/Sdboyer/deptest)
this seems like an incomplete thought? i don't know what the "modification" here is.
making this a requirement is what makes the problem untenable.
at most, i think we can say it makes the problem _frustrating_. if it were so critically flawed an approach as to be unrealistic, then...well, a bit facetious, but we'd hear about it in TIOBE analyses about Go's viability. what we know is that the check exists in the compiler today, and the actual felt pain from it here, as reflected in this issue, seems to be a confluence of two factors:
it is, of course, not impossible that this might happen again in the future. but it strikes me as more of an edge cases, having required a bit of a perfect storm to really have bite.
Adding this as a constant factor in the SAT solver seems like a waste, when the package name could be canonicalized to
github.com/sdboyer/deptestbefore adding the case variations to the solution space.
so the proposal is that the compiler only ever sees the folded/canonicalized paths, and that's the information it has when it goes searching on local disk for files to satisfy an import?
Adding this as a constant factor in the SAT solver seems like a waste, when the package name could be canonicalized to
github.com/sdboyer/deptestbefore adding the case variations to the solution space.
so, to be clear, as a practical matter, this _has_ to happen in dep right now, as it's the only solution we have to fix the pain that exists with currently released versions of the toolchain. we can consider taking it out later if a change like you're proposing lands, but it'd probably have to be quite far in the future. dep has to care about a longer span of time than your average project.
@sdboyer That is a closer interpretation of what I'm saying.
I think the problem with the dep solution is that it won't actually be able to solve the problem if dependencies don't agree on case. The situation is no better or worse than it is today, except we might get a better error message.
Either way, let's shift venues. I'm not sure this is the right place to discuss. I haven't found an issue on golang for this, but it is likely that one exists.
Most helpful comment
Fixed in master. Let me know if this works for you. Thanks for providing adequate context.