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

dune_file_watcher: do not exclude _opam and _esy systematically #7086

Closed
wants to merge 2 commits into from

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Feb 15, 2023

As discussed in the thread starting at #7010 (comment), it seems that it is not right to systematically exclude _opam (and _esy) from the file notification watchers, since these directories may be added explicitly to the Dune build (eg by using (vendored_dirs ...)).

cc @jonahbeckford @rgrinberg

nojb added 2 commits February 15, 2023 14:31
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@jonahbeckford
Copy link
Collaborator

Could we just have a command line option / dune-project config item / environment variable for “don’t use default exclusion rules”? I think that Boolean would be simplest.

But perhaps we can let the user specify the exact exclusion rules as a list of strings (ex. semicolon separated list of rules) instead. Maybe I’m the only one in the world, but I have huge node_modules/ directories inside my source tree when I get Dune to generate JS. And large Conda (Python) environments when I mix OCaml with Sphinx.

@rgrinberg
Copy link
Member

I also think that removing _opam and _esy from the default list should be opt-in. Many users have a local switch in _opam so it doesn't make sense to inconvenience the majority. A command line option or a workspace option to remove the default paths would make more sense.

@jonahbeckford
Copy link
Collaborator

I don't know my way around the dune codebase. This could be something I can take over though if someone could point me to an example in the code of using a workspace option.

@rgrinberg
Copy link
Member

To start, you can just make the patterns configurable. The fact that they're hard coded now means that there's now way to customize them from the code at all. Once you do that, you can add the appropriate option to dune_rules/workspace.ml

@nojb
Copy link
Collaborator Author

nojb commented Feb 16, 2023

Being able to customize the set of exclusion patterns seems the better way to go, so I'll close this one.

@nojb nojb closed this Feb 16, 2023
@nojb nojb deleted the _opam branch February 16, 2023 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants