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

fix(engine): remove old target even if it is a dir #9327

Merged
merged 14 commits into from
Feb 27, 2024
Merged

fix(engine): remove old target even if it is a dir #9327

merged 14 commits into from
Feb 27, 2024

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Nov 30, 2023

Fixes #6575

@Alizter

This comment was marked as outdated.

Sign in to view
@emillon

This comment was marked as outdated.

Sign in to view
@Alizter

This comment was marked as outdated.

Sign in to view
@rgrinberg
Copy link
Member

I see we handle the case when a directory is changed to a file, what about when a file is changed to a directory?

@emillon
Copy link
Collaborator Author

emillon commented Dec 4, 2023

what about when a file is changed to a directory

Because we use rm_rf for directories, I think that it's going to work fine, but it's worth adding a test case.

@emillon
Copy link
Collaborator Author

emillon commented Dec 4, 2023

I extracted a function and amended the test to cover that behavior.

@emillon emillon force-pushed the fix-6575 branch 2 times, most recently from 8bc5a44 to ef89dbf Compare December 22, 2023 14:28
@emillon
Copy link
Collaborator Author

emillon commented Feb 6, 2024

ping @snowleopard - I renamed the functions as requested, I think this is ready to go but let me know if you need to test this.

snowleopard
snowleopard approved these changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

@snowleopard
Copy link
Collaborator

cc @rleshchinskiy who recently implemented something similar internally (I believe he simply called rm_rf in both cases).

@emillon
fix(engine): remove old target even if it is a dir
74304ab 
Fixes ocaml#6575

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Add changelog entry
b06cdcf 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Ignore EPERM on darwin
517d07a 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Extract Fpath.unlink_status
e6dfb29 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Change file to directory
fc58201 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Keep exceptions
f688b2d 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Use a single constructor for exn
4254813 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Log error
eb7402c 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
rename Fpath.unlink to Fpath.unlink_exn
7aa34ed 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
rename Path.unlink to Path.unlink_exn
0de31ef 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
rename Fpath.unlink_status to Fpath.unlink
abbf691 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
rename Path.unlink_status to Path.unlink
e9e05f1 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
do not log error if file does not exist
1f41c74 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Copy link
Collaborator Author

emillon commented Feb 8, 2024

thanks. I pushed a change in order not to log if the file does not exist (which is what was breaking the coq test)

@emillon emillon merged commit f13d9a8 into ocaml:main Feb 27, 2024
26 of 27 checks passed
@emillon emillon deleted the fix-6575 branch February 27, 2024 10:29
@emillon emillon mentioned this pull request Mar 4, 2024
Closed
15 tasks
Leonidas-from-XIV pushed a commit to Leonidas-from-XIV/dune that referenced this pull request Mar 4, 2024
@emillon @Leonidas-from-XIV
fix(engine): remove old target even if it is a dir (ocaml#9327)
0b0ea22 
* fix(engine): remove old target even if it is a dir

Fixes ocaml#6575

Signed-off-by: Etienne Millon <me@emillon.org>

* Add changelog entry

Signed-off-by: Etienne Millon <me@emillon.org>

* Ignore EPERM on darwin

Signed-off-by: Etienne Millon <me@emillon.org>

* Extract Fpath.unlink_status

Signed-off-by: Etienne Millon <me@emillon.org>

* Change file to directory

Signed-off-by: Etienne Millon <me@emillon.org>

* Keep exceptions

Signed-off-by: Etienne Millon <me@emillon.org>

* Use a single constructor for exn

Signed-off-by: Etienne Millon <me@emillon.org>

* Log error

Signed-off-by: Etienne Millon <me@emillon.org>

* rename Fpath.unlink to Fpath.unlink_exn

Signed-off-by: Etienne Millon <me@emillon.org>

* rename Path.unlink to Path.unlink_exn

Signed-off-by: Etienne Millon <me@emillon.org>

* rename Fpath.unlink_status to Fpath.unlink

Signed-off-by: Etienne Millon <me@emillon.org>

* rename Path.unlink_status to Path.unlink

Signed-off-by: Etienne Millon <me@emillon.org>

* do not log error if file does not exist

Signed-off-by: Etienne Millon <me@emillon.org>

---------

Signed-off-by: Etienne Millon <me@emillon.org>
@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request Mar 4, 2024
Merged
Leonidas-from-XIV added a commit that referenced this pull request Mar 5, 2024
@Leonidas-from-XIV @emillon
fix(engine): remove old target even if it is a dir (#9327) (#10204)
10e0155 
* fix(engine): remove old target even if it is a dir

Fixes #6575



* Add changelog entry



* Ignore EPERM on darwin



* Extract Fpath.unlink_status



* Change file to directory



* Keep exceptions



* Use a single constructor for exn



* Log error



* rename Fpath.unlink to Fpath.unlink_exn



* rename Path.unlink to Path.unlink_exn



* rename Fpath.unlink_status to Fpath.unlink



* rename Path.unlink_status to Path.unlink



* do not log error if file does not exist



---------

Signed-off-by: Etienne Millon <me@emillon.org>
Co-authored-by: Etienne Millon <me@emillon.org>
@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request Mar 11, 2024
Closed
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Mar 11, 2024
@Leonidas-from-XIV
[new release] dune (15 packages) (3.14.1)
8ea501e 
CHANGES:

### Fixed

- When a directory is changed to a file, correctly remove it in subsequent
  `dune build` runs. (ocaml/dune#9327, fix ocaml/dune#6575, @emillon)

- Fix a problem with the doc-new target where transitive dependencies were
  missed during compile. This leads to missing expansions in the output docs.
  (ocaml/dune#9955, @jonludlam)

- coq: fix performance regression in coqdep unescaping (ocaml/dune#10115, fixes ocaml/dune#10088,
  @ejgallego, thanks to Dan Christensen for the report)

- coq: memoize coqdep parsing, this will reduce build times for Coq users, in
  particular for those with many .v files (ocaml/dune#10116, @ejgallego, see also ocaml/dune#10088)

- on Windows, use an unicode-aware version of `CreateProcess` to avoid crashes
  when paths contains non-ascii characters. (ocaml/dune#10212, fixes ocaml/dune#10180, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snowleopard snowleopard snowleopard approved these changes

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clean needed when turning a directory into a file
4 participants
@emillon @Alizter @rgrinberg @snowleopard