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.

Sign up for GitHub

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

Jump to bottom

Allow whitespace in target selectors #10032

Merged
merged 1 commit into from
May 26, 2024
Merged

Allow whitespace in target selectors #10032

merged 1 commit into from
May 26, 2024

Conversation

bacchanalia
Copy link
Collaborator

@bacchanalia bacchanalia commented May 19, 2024 •
edited
Loading

Allow spaces in the final component of target selectors. This resolves an issue where using absolute paths in selectors can fail if there is whitespace in the parent directories of the project.

Fixes #8875

@bacchanalia
Copy link
Collaborator Author

This PR allows whitespace to appear in paths, but does not also allow ':''s as discussed in the issue. I made an attempt to also allow ':''s, but it was failing on the Cabal-lib side, for reasons that were not completely clear to me, but probably related to search paths.

@bacchanalia
Copy link
Collaborator Author

I should add a test to cabal-tests also.

@geekosaur
Copy link
Collaborator

Yes.

Don't worry about the changelogs failure; that's a GitHub change we're still sorting out, they bumped the version of ghc included in their build images and some of the transitive dependencies of our tools aren't yet compatible with it.

@bacchanalia
Copy link
Collaborator Author

added a PackageTest

@bacchanalia
Copy link
Collaborator Author

I'm confused why the the test that failed on osx even ran. It looks like it should be marked skip?

@mouse07410
Copy link
Collaborator

I'm coming late to this issue, so please bear with me.

Why should the failing test be skipped on MacOS?
And are there tests that check if a path separated by ":" works?

@bacchanalia
Copy link
Collaborator Author

bacchanalia commented May 19, 2024 •
edited
Loading

@mouse07410 Unrelated test, with "skip" in the test body
https://github.com/haskell/cabal/actions/runs/9146300664/job/25146310082?pr=10032#step:19:427
https://github.com/bacchanalia/cabal/blob/714acec876ea42ad9233c3cd1f7fa8f85a63413b/cabal-testsuite/PackageTests/NewUpdate/RejectFutureIndexStates/cabal.test.hs#L6

fendor
fendor approved these changes May 19, 2024
View reviewed changes
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking care of this!

@bacchanalia
Copy link
Collaborator Author

bacchanalia commented May 19, 2024 •
edited
Loading

I made a PR to properly skip the flaky tests that this PR has been hitting in CI: #10035 (they are supposed to be skipped, but skip is only called after the part that's flaky)

@bacchanalia bacchanalia added merge me Tell Mergify Bot to merge and removed merge me Tell Mergify Bot to merge labels May 20, 2024
@bacchanalia
Copy link
Collaborator Author

question about merging: should I use merge me or squash + merge me or manually squash because the auxiliary commit messages are superficial?

@ulysses4ever
Copy link
Collaborator

squash seems more appropriate to me

@bacchanalia bacchanalia force-pushed the 8875-whitespace-in-target branch from 714acec to 0ac648b Compare May 23, 2024 02:53
@bacchanalia bacchanalia added the merge me Tell Mergify Bot to merge label May 23, 2024
@bacchanalia bacchanalia force-pushed the 8875-whitespace-in-target branch from 0ac648b to 30c6138 Compare May 24, 2024 01:07
@bacchanalia
Copy link
Collaborator Author

rebased to pick up the test fixes

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 26, 2024
@bacchanalia @Mikolaj
Allow whitespace in target selectors
564b4fe 
Disallowing whitespace while parsing target selectors incorrectly
disallows file targets with whitespace in the paths, which can issues
when users pass absolute paths.

fixes haskell#8875
@Mikolaj Mikolaj force-pushed the 8875-whitespace-in-target branch from 30c6138 to 564b4fe Compare May 26, 2024 01:08
@mergify mergify bot merged commit 94277d1 into haskell:master May 26, 2024
50 checks passed
@ulysses4ever
Copy link
Collaborator

@mergify backport 3.12

@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 7, 2024 •
edited
Loading

backport 3.12

✅ Backports have been created

@mergify mergify bot mentioned this pull request Jun 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fendor fendor fendor approved these changes

@geekosaur geekosaur geekosaur approved these changes

Assignees
No one assigned
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cabal commands do not work with whitespace in path
6 participants
@bacchanalia @geekosaur @mouse07410 @ulysses4ever @fendor @Mikolaj