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

Ignore tags files in --watch mode #5352

Closed
wants to merge 2 commits into from
Closed

Ignore tags files in --watch mode #5352

wants to merge 2 commits into from

Conversation

vphantom
Copy link
Contributor

(First-time PR here. 🤞 ) This is more of a suggestion than an improvement, but it would perfectly solve my problem with watch mode until big issues like #2209 and #3579 get tackled.

I use vim-gutentags in combination with the popular tool ctags to get a "tagbar" of files' structures on demand. This means that Vim triggers the reconstruction of the current projects' /tags file every time a file is saved, and this process briefly creates /tags.lock and /tags.temp files. This causes two problems. First, every time I save an OCaml file, Dune rebuilds my project twice in a row because of the tags file and, I assume, the slight delay between the ML file and the tags file being touched. Second, sometimes Dune is woken up by just the tags file operations when I work on non-ML files and:

File "dune", line 1, characters 0-0:
Error: File unavailable: tags.lock
File "dune", line 1, characters 0-0:
Error: File unavailable: tags.temp

Short of fully understanding the syntax available for exclusions in create_fsevents, I added all three names. If globs were allowed I could see something more general like *.lock, *.swp and what not, but I suspect that some of the back-ends might only support exact names.

(Unrelated: there was no final EOL in .gitignore but I omitted that change to keep this PR "clean". Not sure why it's missing though.)

Signed-off-by: Stéphane Lavergne <lis@imars.com>
@vphantom
Copy link
Contributor Author

I don't know anything about GitHub Actions but the macos-latest failure seems unrelated, something about not being the right version of the O/S itself.

@bobot
Copy link
Collaborator

bobot commented Jan 24, 2022

I think tags is too generic, but it is not my part. @aalekseyev

@aalekseyev
Copy link
Collaborator

It also feels a bit too generic to me, the ideal fix should be some combination of:

  • configurable ignore pattern (maybe an option to read patterns from from gitignore)
  • not retriggering build when a file changes that we never looked at

That said, my opinion should not be weighed heavily since I'm not working on dune anymore.

@vphantom
Copy link
Contributor Author

I think by "generic" you actually mean "specific" since this, like the exclusions already hard-coded in dune_file_watcher.ml ("_esy"; "_opam"; ".git"; ".hg";), are specific to Esy, Opam, Git and Mercurial respectively and this would add Ctags to the list. 😉 I would not have proposed this tweak if there hadn't already been one in place, ready to be added to.

This is intended as a "quick fix" until Dune can actually ignore files it doesn't care about (#2209 #3579) which seems to be a complicated issue scheduled for version 3 since it involves several different notification back-ends.

@aalekseyev
Copy link
Collaborator

I think the objection is that the word "tags" is not specific enough, it might be used in Ctags (which is fine), but it might also be used for something else (which could potentially be used in a build).

@rgrinberg
Copy link
Member

Yeah, this does not seem like a thing that we should without letting the user customize it.

I am bit suspicious of this whole approach though. Why don't we just make dune understand .gitignore files and then it will not be necessary to replicate this information in two places.

@vphantom
Copy link
Contributor Author

Supporting .gitignore would be a poor fit: while it's true that I don't commit my tags there's plenty of files that Dune doesn't care about which do make it into my repos (Markdown files, configuration files, etc.) We'd need either a .duneignore file or a new way to tell Dune in existing configuration files (I only know of /dune but I think there's a couple hyphenated ones as well).

I don't even know if exclusion_paths supports GLOBs or represents exact file names, but given that there are several notification back-ends, I suspect that they have to be exact file names. I doubt that just loading each line of a .duneignore file into that list (with the current one as a default when the file is missing) would be acceptable to the Dune maintainers, and I don't know how I'd do GLOB extension there, nor how that might make the list bloated enough to cause performance issues. So… I made this quick fix until a real solution can be worked on carefully later.

If there is any way that a file named tags could be relevant for Dune in other projects, as you fear, then maybe we could at least keep tags.temp and tags.lock in this PR? That would already lessen the problem a bit.

@rgrinberg
Copy link
Member

Note that this problem is no longer acute on master anyhow. In master, dune will receive and then immediately discarded useless notifications. Your build will not be needlessly re-triggered.

@rgrinberg
Copy link
Member

Though you will still see these useless errors.

@vphantom
Copy link
Contributor Author

When you say master do you mean version 3 or sooner in a 2.9.2?

Also about the errors, that seems to mean that we still need to tell Dune to ignore some files explicitly, right?

@vphantom
Copy link
Contributor Author

Wow that file changed a lot in the last few days, you guys have exclude_patterns now. I'm not sure what the # means in there, do you exclude all dot-files and dot-dirs? If so, I'm sure I could tell Vim to tell Ctags to use /.tags instead of /tags to solve my short-term problem (the double building and the vanishing file errors).

Longer-term, are there already plans to extract that list of exclusions into a stanza or a .duneignore kind of file? I was just about to offer help in doing that but it looks like you're actively working on that already. 😬

@rgrinberg rgrinberg added the requires-team-discussion This topic requires a team discussion label Feb 2, 2022
@rgrinberg
Copy link
Member

The fix in this PR is wrong, but we need to discuss what we're planning to do about this in the next meeting.

@rgrinberg
Copy link
Member

From today's discussion:

  • We agree that there should be a mechanism to ignore files
  • The mechanism should make it easy to specify a mask for all directories
  • It should subsume our current dirs stanza.

@rgrinberg rgrinberg removed the requires-team-discussion This topic requires a team discussion label Feb 9, 2022
@rgrinberg
Copy link
Member

This ticket asks for a similar feature: #4865 so let's continue the discussion there.

@rgrinberg rgrinberg closed this Feb 9, 2022
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.

4 participants