Case change breaks builds? #451
Comments
|
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 $ 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 #384.
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: #384 (comment) 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.. 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 |
|
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 |
|
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 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: After running that, the build works fine. This is probably not a "production" solution, but neither is "go get". |
|
@stevvooe osrg/gobgp#1182 |
|
@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 |
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 |
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: osrg/gobgp#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 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 @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 #384 (comment), in were he suggests this to be continued in This ensures support for those vendoring, or However - I feel a repo change shouldn't be purely on a naming and conventional basis, but should have a larger driving factor. |
|
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 |
|
@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: golang/dep#433 (comment) 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. |
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...
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: golang/dep#433 (comment). quoting from there:
this is the first-class check in the compiler that produces the error.
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 |
|
@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:
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 #451 (comment)). There is no point in making the 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 |
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.
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.
yes. agreed. and that's what the changes to dep in that PR are enforcing.
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."
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:
but maybe i'm still not understanding, and that's not what you mean? |
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 Case-folding import paths would not have an effect existing Go code. No one expects 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 |
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.
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:
this seems like an incomplete thought? i don't know what the "modification" here is.
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.
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?
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 Either way, let's shift venues. I'm not sure this is the right place to discuss. I haven't found an issue on |
See: sirupsen/logrus@202f255 * Officially changed name to lower-case And: sirupsen/logrus@0afea37 sirupsen/logrus#451 (comment)
* Use github.com/sirupsen/logrus, rather than github.com/Sirupsen/logrus See: sirupsen/logrus@202f255 * Officially changed name to lower-case And: sirupsen/logrus@0afea37 sirupsen/logrus#451 (comment) * Pin against a stable version of github.com/urfave/cli * Move to govendor * Run govendor sync, and check in vendored code
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
Stosin 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.WithFieldsAny tips appreciated.
The text was updated successfully, but these errors were encountered: