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

avoid invalid filepaths on Windows #9254

Merged
merged 2 commits into from
Sep 28, 2023
Merged

avoid invalid filepaths on Windows #9254

merged 2 commits into from
Sep 28, 2023

Conversation

geekosaur
Copy link
Collaborator

Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, etc.)

Include the following checklist in your PR:

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Thanks!

@Mikolaj
Copy link
Member

Mikolaj commented Sep 13, 2023

Would you mind providing a link that, more or less, captures the rules you are using? Preferably with a version number or date of retrieval.

I've found something related:

Cabal-tests/tests/ParserTests/regressions/denormalised-paths.check:The paths 'files/<>/.txt', 'c/**/.c', 'C:foo/bar', '||s' are invalid on Windows, which would cause portability problems for this package. Windows file names cannot contain any of the characters ":*?<>|" and there a few reserved names including "aux", "nul", "con", "prn", "com1-9", "lpt1-9" and "clock$".

and

Cabal/ChangeLog.md- * With build-type: configure, avoid using backslashes to delimit
Cabal/ChangeLog.md: path components on Windows and warn about other unsafe characters
Cabal/ChangeLog.md- in the path to the source directory on all platforms
Cabal/ChangeLog.md- (#5386).

and

Cabal/src/Distribution/PackageDescription/Check.hs: [ check (not . FilePath.Windows.isValid . prettyShow . packageName $ pkg)

@geekosaur
Copy link
Collaborator Author

https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file is where I got the list I mentioned here.

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Likely sound (excluding bad names), certainly not complete (excluding only bad names).
Maybe ok, since there are enough good names it let's through. ;-)

Cabal-QuickCheck/src/Test/QuickCheck/Instances/Cabal.hs Outdated Show resolved Hide resolved
Cabal-QuickCheck/src/Test/QuickCheck/Instances/Cabal.hs Outdated Show resolved Hide resolved
Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks!

@andreasabel andreasabel added the squash+merge me Tell Mergify Bot to squash-merge label Sep 26, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 28, 2023
@mergify mergify bot merged commit d363088 into master Sep 28, 2023
45 checks passed
@mergify mergify bot deleted the windows-qc branch September 28, 2023 07:20
erikd pushed a commit to erikd/cabal that referenced this pull request Oct 3, 2023
* avoid invalid filepaths on Windows

* comment bad Arbitrary instances
erikd pushed a commit to erikd/cabal that referenced this pull request Oct 3, 2023
* avoid invalid filepaths on Windows

* comment bad Arbitrary instances
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QuickCheck PathName function may produce bad paths on Windows
5 participants