Skip to content

Support recursive globs in file monitoring (#10064)#11658

Merged
mergify[bot] merged 1 commit intohaskell:masterfrom
tdammers:wip/tobias-10064
May 2, 2026
Merged

Support recursive globs in file monitoring (#10064)#11658
mergify[bot] merged 1 commit intohaskell:masterfrom
tdammers:wip/tobias-10064

Conversation

@tdammers
Copy link
Copy Markdown
Contributor

@tdammers tdammers commented Mar 25, 2026

Implements support for recursive globs (some/path/**/file.name) for file monitoring.

QA Notes

Previously, attempting to use recursive globs (**) to declare build rules (with build-type "Hooks") would yield an error message saying that this isn't implemented; this should now work. See #10064 for details and examples.

@philderbeast
Copy link
Copy Markdown
Collaborator

Will this resolve #10064?

@tdammers
Copy link
Copy Markdown
Contributor Author

Will this resolve #10064?

Yes, that is the idea.

Copy link
Copy Markdown
Collaborator

@sheaf sheaf left a comment

Choose a reason for hiding this comment

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

This is looking great, but I think the testing strategy is insufficient. It's quite awkward to go through SetupHooks for testing; I'm not sure whether there's a better (and more direct) way to test the code. We definitely need to test more things:

  • recurring deeper into subdirectories of subdirectories
  • making sure that we don't monitor things that we shouldn't:
    • adding a directory which contains no files that match
    • modifying a file that doesn't match at any level of the directory hierarchy

Again it's not great to test this within the SetupHooks framework as it muddies the waters (and unrelated bugs may interfere).

I'm also wondering whether we need to take into account possible recursive symlinks?
If we have foo/bar/baz which is a symlink back to foo, then we could enter an infinite loop here. So perhaps we need to additionally keep track of which directories we have seen to avoid entering them again?

Comment thread cabal-install/src/Distribution/Client/FileMonitor.hs
Comment thread cabal-install/src/Distribution/Client/FileMonitor.hs Outdated
Comment thread cabal-install/src/Distribution/Client/FileMonitor.hs
Comment thread cabal-install/src/Distribution/Client/FileMonitor.hs
Comment thread cabal-install/src/Distribution/Client/FileMonitor.hs
Comment thread cabal-install/src/Distribution/Client/FileMonitor.hs
Comment thread cabal-install/src/Distribution/Client/FileMonitor.hs
Comment thread cabal-testsuite/PackageTests/SetupHooks/SetupHooksRecursiveGlob/setup.out Outdated
@tdammers
Copy link
Copy Markdown
Contributor Author

This is looking great, but I think the testing strategy is insufficient. It's quite awkward to go through SetupHooks for testing; I'm not sure whether there's a better (and more direct) way to test the code. We definitely need to test more things:

* recurring deeper into subdirectories of subdirectories

* making sure that we don't monitor things that we shouldn't:
  
  * adding a directory which contains no files that match
  * modifying a file that doesn't match at any level of the directory hierarchy

Probably also:

  • Adding/removing entire new subdirectories (and subdirectories of subdirectories)
  • Moving files between sub(-sub-sub-...)directories
  • Changing a directory name such that files below it that would previously match no longer do (and also the reverse)

Again it's not great to test this within the SetupHooks framework as it muddies the waters (and unrelated bugs may interfere).

Indeed, I found this test quite awkward to write. Do you have any pointers for me where/how this could better fit into the existing testing framework?

I'm also wondering whether we need to take into account possible recursive symlinks? If we have foo/bar/baz which is a symlink back to foo, then we could enter an infinite loop here. So perhaps we need to additionally keep track of which directories we have seen to avoid entering them again?

Good point, we should probably do that. The alternative would be to just not support symlinks, but that doesn't sound like a nice thing to do to me.

@sheaf
Copy link
Copy Markdown
Collaborator

sheaf commented Mar 30, 2026

Indeed, I found this test quite awkward to write. Do you have any pointers for me where/how this could better fit into the existing testing framework?

It seems there are unit tests for file monitoring in cabal-install/tests/UnitTests/Distribution/Client/FileMonitor. Adding the tests we discussed above sounds like a great plan, so we can make sure the file monitoring logic is correct before we investigate the potential recompilation issue with SetupHooks.

@tdammers tdammers force-pushed the wip/tobias-10064 branch 2 times, most recently from b97a1ca to 4fcdd0d Compare April 15, 2026 09:00
@tdammers tdammers marked this pull request as ready for review April 15, 2026 09:02
@sheaf
Copy link
Copy Markdown
Collaborator

sheaf commented Apr 27, 2026

This looks excellent. As far as I can see, all that's left to do is to rebase, squash the commits, and update the expected output for the SetupHooksRecursiveGlob test.

Comment thread cabal-testsuite/PackageTests/SetupHooks/SetupHooksRecursiveGlob/setup.out Outdated
Comment thread Cabal/src/Distribution/Simple/Glob/Internal.hs
Comment thread changelog.d/pr-11658.md Outdated
@tdammers tdammers force-pushed the wip/tobias-10064 branch 3 times, most recently from 0f2d6d0 to 894f7a9 Compare April 29, 2026 08:09
@sheaf sheaf added the merge me Tell Mergify Bot to merge label Apr 29, 2026
@mergify mergify Bot added the ready and waiting Mergify is waiting out the cooldown period label Apr 29, 2026
@sheaf sheaf removed merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period labels Apr 29, 2026
Comment thread cabal-testsuite/PackageTests/SetupHooks/SetupHooksRecursiveGlob/setup.test.hs Outdated
@sheaf sheaf added the merge me Tell Mergify Bot to merge label Apr 30, 2026
@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 queued labels Apr 30, 2026
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented May 2, 2026

Merge Queue Status

  • Entered queue2026-05-02 10:17 UTC · Rule: default
  • Checks skipped · PR is already up-to-date
  • Merged2026-05-02 10:27 UTC · at 53d2cda31926f6547329405b51a0f86efc8f6a2b · merge

This pull request spent 10 minutes 25 seconds in the queue, including 1 second running CI.

Required conditions to merge
  • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
  • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
  • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
  • any of [🛡 GitHub branch protection]:
    • check-success = Doctest Cabal
    • check-neutral = Doctest Cabal
    • check-skipped = Doctest Cabal
  • any of [🛡 GitHub branch protection]:
    • check-success = Meta checks
    • check-neutral = Meta checks
    • check-skipped = Meta checks
  • any of [🛡 GitHub branch protection]:
    • check-success = docs/readthedocs.org:cabal
    • check-neutral = docs/readthedocs.org:cabal
    • check-skipped = docs/readthedocs.org:cabal
  • any of [🛡 GitHub branch protection]:
    • check-success = Validate post job
    • check-neutral = Validate post job
    • check-skipped = Validate post job
  • any of [🛡 GitHub branch protection]:
    • check-success = fourmolu
    • check-neutral = fourmolu
    • check-skipped = fourmolu
  • any of [🛡 GitHub branch protection]:
    • check-success = hlint
    • check-neutral = hlint
    • check-skipped = hlint
  • any of [🛡 GitHub branch protection]:
    • check-success = Bootstrap post job
    • check-neutral = Bootstrap post job
    • check-skipped = Bootstrap post job
  • any of [🛡 GitHub branch protection]:
    • check-success = whitespace
    • check-neutral = whitespace
    • check-skipped = whitespace
  • any of [🛡 GitHub branch protection]:
    • check-success = Check sdist post job
    • check-neutral = Check sdist post job
    • check-skipped = Check sdist post job
  • any of [🛡 GitHub branch protection]:
    • check-success = Changelogs
    • check-neutral = Changelogs
    • check-skipped = Changelogs

@mergify mergify Bot merged commit 309fd28 into haskell:master May 2, 2026
201 of 208 checks passed
@mergify mergify Bot removed the queued label May 2, 2026
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.

4 participants