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

Renaming upper-case 'Sirupsen' to 'sirupsen' #384

Merged
merged 4 commits into from
Nov 30, 2016
Merged

Renaming upper-case 'Sirupsen' to 'sirupsen' #384

merged 4 commits into from
Nov 30, 2016

Conversation

tobbbles
Copy link
Contributor

@tobbbles tobbbles commented Jul 18, 2016

The reason for this is that most other packages use lower-case naming conventions.

The vendoring tool I use (GB) is case sensitive, and can become a massive pain in the 🍑 when Logrus is the only package with an upper-case name (as most other packages are in lower case, following https://blog.golang.org/package-names) - this often leads to build failures.

Why don't you just vendor it in lower case?

Because Logrus has itself as an internal dependency, it vendors two folders, Sirupsen/logrus and sirupsen/logrus.

@aarongreenlee aarongreenlee self-assigned this Jul 27, 2016
@aarongreenlee
Copy link
Collaborator

aarongreenlee commented Jul 27, 2016

Thank you for your contribution, @mnzt.

Tools like go get seem to be case insensitive. I wonder if GB would also benefit by becoming case insensitive.

I don't see any negative side effects to this pull request and will accept it as soon as the tests can pass.

Thanks again.

@tobbbles
Copy link
Contributor Author

@aarongreenlee thank you for taking time to review the PR 😄.

It looks like a linter tool automatically removed the airbrake import, whoops!

I've noticed that logrus examples imports airbreak, which in turn imports logrus - to circumvent this issue I have added a build constraint flag. Hopefully all goes well on the following CI check 😸

@@ -1,8 +1,14 @@
// +build ignore
// Do NOT include the above line in your code. This is a build contrainst used
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"constraint"

@glasser
Copy link
Contributor

glasser commented Aug 6, 2016

Does it make sense to use Go 1.4 custom import path checking for this? https://docs.google.com/document/d/1jVFkZTcYbNLaTxXD9OcGfn7vYv5hWtPx9--lTx1gPMs/edit

@tobbbles
Copy link
Contributor Author

@glasser I think the main issue here was that an import cycle was created in examples/hook.

@tobbbles
Copy link
Contributor Author

Any progress on this @aarongreenlee?

@stevvooe
Copy link
Collaborator

@sirupsen This looks like a real issue but it would be a possibly breaking change.

Any opinion here?

@sirupsen
Copy link
Owner

sirupsen commented Nov 18, 2016

How do you think this could break @stevvooe? This just changes the copy/paste examples to use lower-case, right?

@stevvooe
Copy link
Collaborator

@sirupsen When this package changes to github.com/sirupsen/logrus, won't the builds fail if the only thing found in GOPATH is github.com/Sirupsen/logrus on a case sensitive filesystem?

For the most part, I am not against this change. If we are willing to risk possible breakage, it is for the better.

@tobbbles
Copy link
Contributor Author

People can still vendor an upper case S in sirupsen, however it's common
practice to keep packages as lower case.

On Fri, 18 Nov 2016, 19:53 Stephen Day, notifications@github.com wrote:

@sirupsen https://github.com/Sirupsen When this package changes to
github.com/sirupsen/logrus, won't the builds fail if the only thing found
in GOPATH is github.com/Sirupsen/logrus on a case sensitive filesystem?

For the most part, I am not against this change. If we are willing to risk
possible breakage, it is for the better.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#384 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJoYeyCn7T3FTfu8oWgjWCFAUr3GT-Rbks5q_gIkgaJpZM4JO2SM
.

@tobbbles
Copy link
Contributor Author

The real issue here is other dependencies vendoring with an upper case S.
Often at work I end up with two vendored lots of logrus - as half use the
common practice, and the over half use what's the in the documentation.

On Fri, 18 Nov 2016, 19:54 Toby Archer, tobyjames1234@googlemail.com
wrote:

People can still vendor an upper case S in sirupsen, however it's common
practice to keep packages as lower case.

On Fri, 18 Nov 2016, 19:53 Stephen Day, notifications@github.com wrote:

@sirupsen https://github.com/Sirupsen When this package changes to
github.com/sirupsen/logrus, won't the builds fail if the only thing found
in GOPATH is github.com/Sirupsen/logrus on a case sensitive filesystem?

For the most part, I am not against this change. If we are willing to risk
possible breakage, it is for the better.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#384 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJoYeyCn7T3FTfu8oWgjWCFAUr3GT-Rbks5q_gIkgaJpZM4JO2SM
.

@stevvooe
Copy link
Collaborator

LGTM

@mnzt Have you seen any breakage?

@sirupsen If you put your LGTM, I think we should go through with this. We can send out some tweets to let people know.

@sirupsen
Copy link
Owner

Let's go ahead with this.

Will it work with this if I rename myself to @sirupsen instead of @sirupsen on Github? It's silly that I have upper-case.

@stevvooe
Copy link
Collaborator

@sirupsen Please, do the honors. I gave up my capital S long ago.

@sirupsen
Copy link
Owner

Just renamed myself 🙈

@sirupsen sirupsen merged commit 42b84f9 into sirupsen:master Nov 30, 2016
@stevvooe
Copy link
Collaborator

💥

@ahmetb
Copy link

ahmetb commented Nov 30, 2016

no more the first dependency listed on Godeps.json 💥

@hilljgo
Copy link

hilljgo commented Nov 30, 2016

Note: this broke our glide dependencies with the following error:

[WARN]	Error updating github.com/sirupsen/logrus: The Remote does not match the VCS endpoint

changing our import from github.com/Sirupsen/logrus to github.com/sirupsen/logrus fixed it

@stevvooe
Copy link
Collaborator

@hilljgo Thanks for the report! Seems like a low impact fix.

@hryx
Copy link

hryx commented Nov 30, 2016

I observed the same Glide issue as @hilljgo, but the problem ended up being exponentially more painful when vendoring several projects which also use Logrus.

If using the old name, Glide fails due to a mismatched remote URL. If using the new name, Go fails due to a case-insensitive import collision.

So the only option I've found, until those vendored projects all start using the lowercase s, is to lock to an older version of Logrus and tell Glide the remote URL explicitly:

- package: github.com/Sirupsen/logrus
  version: a437dfd2463eaedbec3dfe443e477d3b0a810b3f
  repo:    https://github.com/sirupsen/logrus

@bbrks
Copy link
Contributor

bbrks commented Dec 1, 2016

Just leaving a comment here for anybody Googling errors.

case-insensitive import collision: "github.com/Sirupsen/logrus" and "github.com/sirupsen/logrus"

The fix is to lowercase all of your import statements for logrus.

One liner recursive find/replace:

find ./ -type f -name "*.go" -exec sed -i -e 's/github.com\/Sirupsen\/logrus/github.com\/sirupsen\/logrus/g' {} \;

Don't forget to run gofmt if your CI pipeline has it as a requirement! Uppercase and lowercase imports will be in a different order.

@tobbbles
Copy link
Contributor Author

tobbbles commented Dec 1, 2016

Note on the above @bbrks' comment, for https://getgb.io you'll need to unvendor (gb vendor delete github.com/Sirupsen/logrus) the uppercase named dependency and then vendor the lowercase named dependency (gb vendor fetch github.com/sirupsen/logrus).

There may be a small teething period of getting everything over to the correct naming - but it'll be worth it in the end

philpennock added a commit to PennockTech/fingerd that referenced this pull request Aug 25, 2017
Switch `github.com/Sirupsen/logrus` → `github.com/sirupsen/logrus`

Rename was handled in sirupsen/logrus#384
in November 2016.
mattbostock added a commit to mattbostock/timbala that referenced this pull request Sep 4, 2017
The logrus package import changed letter case:
sirupsen/logrus#384

In c31b012 I opted to use the same case as Prometheus packages I depend
on to avoid case conflicts.

I'm about to start using Go's dep tool for dependency management, and
the old logrus import prevents dep from completing its initialisation
because newer Prometheus packages use the lowercase import path:
golang/dep#797

Use the lowercase import path for consistency with the Prometheus
packages.
mattbostock added a commit to mattbostock/timbala that referenced this pull request Sep 4, 2017
The logrus package import changed letter case:
sirupsen/logrus#384

In c31b012 I opted to use the same case as Prometheus packages I depend
on to avoid case conflicts.

I'm about to start using Go's dep tool for dependency management, and
the old logrus import prevents dep from completing its initialisation
because newer Prometheus packages use the lowercase import path:
golang/dep#797

Use the lowercase import path for consistency with the Prometheus
packages.
muesli added a commit to muesli/dep that referenced this pull request Sep 4, 2017
Use the new canonical project URL (note the lowercase 's') for logrus to prevent duplicate checkouts & imports of logrus.

See: sirupsen/logrus#384
Paddy-Truck pushed a commit to Paddy-Truck/logrus that referenced this pull request Sep 8, 2017
This reverts commit 42b84f9, reversing
changes made to cf60a8c.
zknill pushed a commit to zknill/dep that referenced this pull request Oct 6, 2017
Use the new canonical project URL (note the lowercase 's') for logrus to prevent duplicate checkouts & imports of logrus.

See: sirupsen/logrus#384
wking added a commit to wking/go-mtree that referenced this pull request Nov 3, 2017
With:

  $ git mv vendor/github.com/{S,s}irupsen
  $ sed -i 's/Sirupsen/sirupsen/g' $(git grep -l Sirupsen)

catching up with the upstream lowercasing [1,2,3,4].  Note the
compatibility issues discussed in [3], some consumers may prefer to
use the old uppercase version until they have time to update their
other Logrus consumers to the new lowercase form.

[1]: https://github.com/sirupsen/logrus/blame/v1.0.3/README.md#L6
[2]: sirupsen/logrus#384
[3]: sirupsen/logrus#570 (comment)
[4]: sirupsen/logrus#553
wking added a commit to wking/go-mtree that referenced this pull request Nov 3, 2017
With:

  $ git mv vendor/github.com/{S,s}irupsen
  $ sed -i 's/Sirupsen/sirupsen/g' $(git grep -l Sirupsen)

catching up with the upstream lowercasing [1,2,3,4].  Because of the
compatibility issues discussed in [3], some consumers may prefer to
use the old uppercase version until they have time to update their
other Logrus consumers to the new lowercase form.

[1]: https://github.com/sirupsen/logrus/blame/v1.0.3/README.md#L6
[2]: sirupsen/logrus#384
[3]: sirupsen/logrus#570 (comment)
[4]: sirupsen/logrus#553
kaedys pushed a commit to kaedys/go-slackbot that referenced this pull request Nov 13, 2017
htfy96 added a commit to sjtug/lug that referenced this pull request Feb 13, 2018
sirupsen/logrus#384

github.com/Sirupsen/logrus => github.com/sirupsen/logrus
flw-cn pushed a commit to flw-cn/go-slackbot that referenced this pull request Jun 25, 2018
cgxxv pushed a commit to cgxxv/logrus that referenced this pull request Mar 25, 2022
Renaming upper-case 'Sirupsen' to 'sirupsen'
cgxxv pushed a commit to cgxxv/logrus that referenced this pull request Mar 25, 2022
This reverts commit 3ad6ae6, reversing
changes made to ac66b27.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.