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

Add module name validation in cabal check command #10816

Merged
merged 1 commit into from
Apr 13, 2025

Conversation

mpickering
Copy link
Collaborator

@mpickering mpickering commented Mar 5, 2025

This change ensures that Cabal warns about modules with names that would cause portability issues, especially on Windows systems where certain names like "aux", "con", "nul", etc. are reserved by the operating system.

Fixes #10295

Please read Github PR Conventions and then fill in one of these two templates.


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:


Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

@mpickering mpickering force-pushed the wip/cabal-check-valid-module-names branch from 27d9dac to e640ed5 Compare March 5, 2025 16:22
@mpickering mpickering force-pushed the wip/cabal-check-valid-module-names branch 2 times, most recently from 012452d to 3824275 Compare March 5, 2025 16:28
@mpickering mpickering force-pushed the wip/cabal-check-valid-module-names branch from 3824275 to c404296 Compare March 5, 2025 19:21
@mpickering
Copy link
Collaborator Author

Thanks for your comments so far, much appreciated. Just let me know how you prefer things to be structured and I will adapt the PR.

@ffaf1
Copy link
Collaborator

ffaf1 commented Mar 5, 2025

Thanks for your comments so far, much appreciated. Just let me know how you prefer things to be structured and I will adapt the PR.

Thanks for the PR! I promise tomorrow (or WE at worst) I will take a thorough look instead of poking bits of it.

@mpickering mpickering force-pushed the wip/cabal-check-valid-module-names branch from c404296 to 5029e1d Compare March 10, 2025 13:42
@mpickering
Copy link
Collaborator Author

I have modified the test like you suggested.

  • Remove assertOutputContains
  • Create empty directories (these need to contain files as the testsuite driver will not create empty directories when the files are copied).

I don't agree with the change to assertOutputContains, I have seen it several times where --accept is used without considering the changed output. It is easier to discern what a test is supposed to do with an explicit assertion. However, I will just note that objection and have changed it as you've requested.

@ffaf1
Copy link
Collaborator

ffaf1 commented Mar 10, 2025

Thanks, I appreciate!

@mpickering
Copy link
Collaborator Author

@ffaf1 Can we merge this patch?

@ffaf1 ffaf1 added attention: needs-help Help wanted with this issue/PR and removed attention: needs-review attention: needs-help Help wanted with this issue/PR labels Apr 10, 2025
@ffaf1
Copy link
Collaborator

ffaf1 commented Apr 10, 2025

We need one more review! Let me readd the label, it should hopefully bring more eyes to this (by popping up on matrix again, that is)!

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.

Stellar!

@mpickering mpickering added the merge me Tell Mergify Bot to merge label Apr 11, 2025
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Apr 11, 2025
This change ensures that Cabal warns about modules with names that would cause
portability issues, especially on Windows systems where certain names like "aux",
"con", "nul", etc. are reserved by the operating system.

Fixes #10295
@Mikolaj Mikolaj force-pushed the wip/cabal-check-valid-module-names branch from 5029e1d to faa3c6a Compare April 13, 2025 10:35
@mergify mergify bot merged commit de66952 into master Apr 13, 2025
55 checks passed
@mergify mergify bot deleted the wip/cabal-check-valid-module-names branch April 13, 2025 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make cabal check reject packages with invalid file names
4 participants