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

x/mod/module: document that non-short Windows names not allowed in module paths #47493

Closed
gazerro opened this issue Aug 1, 2021 · 5 comments
Closed
Labels
Documentation FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@gazerro
Copy link
Contributor

gazerro commented Aug 1, 2021

The CheckPath, CheckImportPath and CheckFilePath functions (used to check module, import and file paths) reject Window short files (as THISIS~1.TXT) but also reject paths like this-is-a-long-file-path~1 and this-is-a-long-file-path~1.txt.

A short file name can have at most 8 characters before the dot and its extension can have at most 3 characters.

@gopherbot gopherbot added this to the Unreleased milestone Aug 1, 2021
@seankhliao
Copy link
Member

This seems to be working as documented/intended?

CheckFilePath passes: https://play.golang.org/p/8u7pObwsxjX

While the other 2 are documented to reject such paths:

CheckImportPath:

The element must not have a suffix of a tilde followed by one or more ASCII digits (to exclude paths elements that look like Windows short-names).

CheckPath:

A valid module path is a valid import path, as checked by CheckImportPath

@seankhliao seankhliao added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 1, 2021
@gazerro
Copy link
Contributor Author

gazerro commented Aug 1, 2021

CheckFilePath passes: https://play.golang.org/p/8u7pObwsxjX

You're right. CheckFilePath does not check for short names. I should have checked better.

While the other 2 are documented to reject such paths:

Yes, they are documented but in my opinion the original commit 4ab6fb1 that introduced this check was too restrictive and rejects path that are as documented but are not short-names in Windows.

@bcmills
Copy link
Contributor

bcmills commented Aug 3, 2021

in my opinion the original commit 4ab6fb1 that introduced this check was too restrictive and rejects path that are as documented but are not short-names in Windows.

Is there a specific reason we would need to allow these paths? (Are there existing widely-used packages that rely on them?)

I agree that the check may be somewhat more conservative than is strictly necessary, but I don't see a significant harm in leaving it that way.

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Aug 3, 2021
@gazerro
Copy link
Contributor Author

gazerro commented Aug 3, 2021

A part from that it may be somewhat more conservative, there is not a specific reason. Having understood that this implementation is a deliberate choice, it is okay for me to close this issue.

However, I recommend mentioning the short-name requirements in the documentation on the page

https://golang.org/ref/mod#go-mod-file-ident

Having to implement a module path validator, without using existing code, I went from this documentation to the code implementing it without reading the CheckPath function comment and so I hadn't read that the behavior was intentional.

@gopherbot
Copy link

Change https://golang.org/cl/344569 mentions this issue: _content/ref/mod: document that module paths can't contain short names

@jayconrod jayconrod changed the title x/mod/module: non-short Windows files rejected in paths x/mod/module: document that non-short Windows names not allowed in module paths Aug 23, 2021
@jayconrod jayconrod added Documentation modules NeedsFix The path to resolution is known, but the work has not been done. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Aug 23, 2021
@golang golang locked and limited conversation to collaborators Aug 24, 2022
passionSeven added a commit to passionSeven/website that referenced this issue Oct 18, 2022
Fixes golang/go#47493

Change-Id: I97d929e646b107766ed44f9bd120ab07e15429e4
Reviewed-on: https://go-review.googlesource.com/c/website/+/344569
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants