[merlin] Cleanup leftover merlin files by voodoos · Pull Request #4261 · ocaml/dune

voodoos · 2021-02-19T12:40:25Z

Would fix #4168

This PR is a proposal to automatically get rid of .merlin files that were promoted by earlier versions of Dune (mainly 2.7.1).

The criteria is:

The file _build/.to-delete-in-source-tree that contains the list of promoted until-clean files has a .merlin entry for the current directory.
No rule declares promoting a file named .merlin in the current directory.

This way, user that setup custom rules to generate custom .merlin won't see there promoted file disapear unexpectedly.
I will test it in real upgrade conditions this afternoon.

I understand this may feel too hacky. If you feel deleting the files that way is too sketchy we always can raise a warning instead, asking for a dune clean.

voodoos · 2021-02-19T15:21:52Z

I can confirm that:

building a project with Dune 2.7.1 generates .merlin files
rebuilding with Dune 2.8.2 does not delete these files
rebuilding with this PR does

test/blackbox-tests/test-cases/promote/merlin-files.t/run.t

rgrinberg · 2021-02-23T04:42:37Z

@aalekseyev mind taking a look at this?

aalekseyev · 2021-02-24T12:15:40Z

Looking.

aalekseyev · 2021-02-24T12:51:01Z

src/dune_engine/build_system.ml

+    let merlin_in_src = Path.Source.(relative source_dir merlin_file) in
+    let source_files_to_ignore =
+      if
+        Path.Set.mem (Promoted_to_delete.load ()) (Path.source merlin_in_src)


Calling load once per directory sounds expensive. My understanding is that we expect this file to have size O(n) where n is the number of directories with .merlin files (eve without taking into account anything else the user happens to promote) and you're loading it n times, which makes for O(n^2) time complexity.

I think we should be able to change it so that load is only called once, and Promoted_to_delete.dump simply writes out the db without calling load again.

src/dune_engine/build_system.ml

aalekseyev · 2021-02-24T13:15:00Z

I just pushed a commit extracting the new code into a separate function and adding some comments.

src/dune_engine/build_system.ml

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

voodoos · 2021-02-25T13:21:24Z

Thanks for your review @aalekseyev.

I modified the load function to read the database from disk only once.
I also took that opportunity change the behavior of remove_now. The updated database (without .merlin) files will be dumped only at the end of the build, not each time a .merlin file is removed.

There are still checks that will run for each folder, do you think it is an issue ?
I am thinking about that test:

dune/src/dune_engine/build_system.ml

Lines 927 to 928 in 63d0a2f

    
           Path.Set.mem (Promoted_to_delete.load ()) (Path.source merlin_in_src) 
        
           && not (Path.Source.Set.mem source_files_to_ignore merlin_in_src)

Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>

aalekseyev · 2021-02-25T14:03:13Z

I pushed a commit simplifying the db management code in Promoted_to_delete. Instead of storing two chunks of the database that need to be merged at some point, I think it's much simpler to store just one database that's already merged. That makes it easy (or at least easier) to reason about watching builds and it seems simpler in general.

Please let me know if I'm not seeing a specific reason to avoid that change.

aalekseyev · 2021-02-25T14:05:40Z

Running a simple test membership check once per directory does not sound problematic to me.

voodoos · 2021-02-25T16:06:21Z

I pushed a commit simplifying the db management code in Promoted_to_delete. [...]

Please let me know if I'm not seeing a specific reason to avoid that change.

I considered doing it myself, I think it is a very reasonable change.
(At first I thought it would make load happen more often than before but it doesn't).

aalekseyev · 2021-02-25T16:23:21Z

There are still checks that will run for each folder, do you think it is an issue ?

I think doing a couple of extra map lookups per directory is fine.

* Make Build_system delete left-over `.merlin` files * simplify code managing the Promoted_to_delete db * Add a test for merlin files cleanup Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com> Co-authored-by: Arseniy Alekseyev <aalekseyev@janestreet.com>

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

* Make Build_system delete left-over `.merlin` files * simplify code managing the Promoted_to_delete db * Add a test for merlin files cleanup Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com> Co-authored-by: Arseniy Alekseyev <aalekseyev@janestreet.com>

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

@hakuch

…ne-action-plugin, dune-private-libs and dune-glob (2.8.3) CHANGES: - Make `patdiff` show refined diffs (ocaml/dune#4257, fixes ocaml/dune#4254, @hakuch) - Fixed a bug that could result in needless recompilation under Windows due to case differences in the result of `Sys.getcwd` (observed under `emacs`). (ocaml/dune#3966, @nojb). - Restore compatibility with Coq < 8.10 for coq-lang < 0.3 , document that `(using coq 0.3)` does require Coq 8.10 at least (ocaml/dune#4224, fixes ocaml/dune#4142, @ejgallego) - Add a META rule for 'compiler-libs.native-toplevel' (ocaml/dune#4175, @AltGr) - No longer call `chmod` on symbolic links (fixes ocaml/dune#4195, @dannywillems) - Dune no longer automatically create or edit `dune-project` files (ocaml/dune#4239, fixes ocaml/dune#4108, @jeremiedimino) - Have `dune` communicate the location of the standard library directory to `merlin` (ocaml/dune#4211, fixes ocaml/dune#4188, @nojb) - Workaround incorrect exception raised by Unix.utimes (OCaml PR#8857) in Path.touch on Windows (ocaml/dune#4223, @dra27) - `dune ocaml-merlin` is now able to provide configuration for source files in the `_build` directory. (ocaml/dune#4274, @voodoos) - Automatically delete left-over Merlin files when rebuilding for the first time a project previously built with Dune `<= 2.7`. (ocaml/dune#4261, @voodoos, @aalekseyev) - Fix `ppx.exe` being compiled for the wrong target when cross-compiling (ocaml/dune#3751, fixes ocaml/dune#3698, @toots) - `dune top` correctly escapes the generated toplevel directives, and make it easier for `dune top` to locate C stubs associated to concerned libraries. (ocaml/dune#4242, fixes ocaml/dune#4231, @nojb) - Do not pass include directories containing native objects when compiling bytecode (ocaml/dune#4200, @nojb)

rand00 · 2021-08-20T11:58:59Z

Not all of my .merlin files were deleted after upgrading to dune.2.9.0, and would have loved a warning from dune that it was using a .merlin file instead of dune ocaml-merlin

voodoos · 2021-08-30T09:12:59Z

Not all of my .merlin files were deleted after upgrading to dune.2.9.0, and would have loved a warning from dune that it was using a .merlin file instead of dune ocaml-merlin

Was the leftover .merlin file created by Dune ? Some edge cases can benefit from using a custom merlin file so user-created ones are not deleted by Dune.

rand00 · 2021-08-30T09:31:47Z

Yes it was by Dune, had a look at it before I deleted it manually. I'm pretty sure that I've never opened that file before, so I wouldn't have edited it accidentally. How does Dune determine if file was created by it?

voodoos · 2021-08-30T09:39:54Z

It keeps a database of promoted until-clean files so that they are deleted when cleaning. If you manually removed your _build directory once this database might not be in sync since leftover .merlin files would be considered user-created when a new build is done after that.

I don't see an easy way to distinguish between this kind of desynchronization and legit user-created .merlin files...

rand00 · 2021-08-30T09:50:58Z

No I can see the problem - though my thought was more in the direction of Dune informing the user after upgrading to a version that switches away from .merlin files, if there were still .merlin files present that it didn't delete.

My primary problem was that I had to debug what was wrong with Merlin without any hints from Dune or Merlin

voodoos added this to the 2.8.3 milestone Feb 19, 2021

voodoos requested review from a user, aalekseyev and rgrinberg February 19, 2021 12:40

rgrinberg reviewed Feb 22, 2021

View reviewed changes

test/blackbox-tests/test-cases/promote/merlin-files.t/run.t Show resolved Hide resolved

voodoos force-pushed the merlin-cleanup branch from 649214f to 1df8e3b Compare February 23, 2021 09:37

aalekseyev reviewed Feb 24, 2021

View reviewed changes

src/dune_engine/build_system.ml Outdated Show resolved Hide resolved

aalekseyev reviewed Feb 25, 2021

View reviewed changes

src/dune_engine/build_system.ml Outdated Show resolved Hide resolved

voodoos and others added 6 commits February 25, 2021 14:16

Make Build_system delete left-over .merlin files

d212ca3

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Add a test for merlin files cleanup

ff54af5

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Programatically create empty direcotry in test

445e60c

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

extract the code into a separate function, add comments

384f890

Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>

fix comment

c0474ee

Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>

Only load once the promoted files database

63d0a2f

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

voodoos force-pushed the merlin-cleanup branch from 1608ce8 to 63d0a2f Compare February 25, 2021 13:17

aalekseyev added 2 commits February 25, 2021 13:49

tweak comments

16053e4

Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>

simplify code managing the Promoted_to_delete db

5b6294e

Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>

aalekseyev approved these changes Feb 25, 2021

View reviewed changes

voodoos merged commit 20df2de into ocaml:main Feb 25, 2021

voodoos added a commit to voodoos/dune that referenced this pull request Feb 25, 2021

Update CHANGES for ocaml#4261 (clean merlin files)

ffe9ccd

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

voodoos added a commit that referenced this pull request Feb 25, 2021

Update CHANGES for #4261 (clean merlin files) (#4285)

ba4809e

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

rgrinberg pushed a commit that referenced this pull request Mar 7, 2021

Update CHANGES for #4261 (clean merlin files) (#4285)

245715b

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

rgrinberg mentioned this pull request Mar 7, 2021

[new release] dune-build-info, dune, dune-configurator, dune-site, dune-action-plugin, dune-private-libs and dune-glob (2.8.3) ocaml/opam-repository#18286

Closed

Conversation

voodoos commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voodoos commented Feb 19, 2021

Uh oh!

Uh oh!

rgrinberg commented Feb 23, 2021

Uh oh!

aalekseyev commented Feb 24, 2021

Uh oh!

aalekseyev Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aalekseyev commented Feb 24, 2021

Uh oh!

Uh oh!

voodoos commented Feb 25, 2021

Uh oh!

aalekseyev commented Feb 25, 2021

Uh oh!

aalekseyev commented Feb 25, 2021

Uh oh!

voodoos commented Feb 25, 2021

Uh oh!

aalekseyev commented Feb 25, 2021

Uh oh!

rand00 commented Aug 20, 2021

Uh oh!

voodoos commented Aug 30, 2021

Uh oh!

rand00 commented Aug 30, 2021

Uh oh!

voodoos commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rand00 commented Aug 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

voodoos commented Feb 19, 2021 •

edited

Loading

voodoos commented Aug 30, 2021 •

edited

Loading