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

Support case-sensitive Go module names #86

Merged

Conversation

VJftw
Copy link
Contributor

@VJftw VJftw commented Mar 21, 2023

using strings.ToLower(mod) removes the ability to support case-sensitive Go modules names, for example github.com/antlr/antlr4/runtime/Go/antlr is currently being converted to github.com/antlr/antlr4/runtime/go/antlr which returns a 404 with the Go Proxy.


I haven't tested this, but can't immediately see a reason for lowercasing the mod. Please share if so, and I'll try to find a better solution :)

using `strings.ToLower(mod)` removes the ability to support case-sensitive Go modules names, for example `github.com/antlr/antlr4/runtime/Go/antlr` is currently being converted to `github.com/antlr/antlr4/runtime/go/antlr` which returns a 404 with the Go Proxy
@Tatskaari
Copy link
Contributor

I think this makes sense, I'm just a little concerned that I added this for a reason... I will have to test this out in a few places before we land this.

@VJftw
Copy link
Contributor Author

VJftw commented Mar 23, 2023

👍 - it does feel like it was done purposefully! Please test away, let me know if I can help

@peterebden
Copy link
Contributor

I recall some nonsense with Sirupsen/logrus and sirupsen/logrus where various things seemed to be inconsistent about it.

I agree that we probably shouldn't be doing this though.

@VJftw
Copy link
Contributor Author

VJftw commented Mar 23, 2023

Ah I see (sirupsen/logrus#543). I think I could probably update this PR to allow end-users to override a module name, e.g. end-users add GoModuleRename=github.com/Sirupsen/logrus=github.com/sirupsen/logrus in .plzconfig.

@peterebden
Copy link
Contributor

I think hopefully we don't need that in the config; i think you could achieve the same with a go_mod_download to instruct it more directly where to get the source from

@Tatskaari
Copy link
Contributor

Maybe we can just try and resolve both? Go already tries a bunch of arbitrary module names to figure out what the module path is so I don't think that's as ugly as it first sounds.

@VJftw
Copy link
Contributor Author

VJftw commented Mar 29, 2023

Maybe we can just try and resolve both? Go already tries a bunch of arbitrary module names to figure out what the module path is so I don't think that's as ugly as it first sounds.

Just updated the PR to do this instead, so it shouldn't break any existing functionality 👍. Let me know what y'all think :)

@Tatskaari
Copy link
Contributor

Thanks for this! This makes a lot of sense.

@VJftw
Copy link
Contributor Author

VJftw commented Apr 5, 2023

you'll need to merge it as I don't have write access 😄

@Tatskaari Tatskaari merged commit 09b4bd1 into please-build:master Apr 6, 2023
@Tatskaari
Copy link
Contributor

you'll need to merge it as I don't have write access smile

Sorry! I was waiting for the build but got distracted. :)

@VJftw VJftw deleted the support-case-sensitive-module-names branch April 6, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants