opam remove --force no longer run commands in current directory
#6672
Conversation
src/client/opamClient.ml
Outdated
| List.fold_left (fun nothing_to_do atom -> | ||
| nothing_to_do && force_remove atom) | ||
| true not_installed |
There was a problem hiding this comment.
This changes the behaviour from "run force_remove for all elements of not_installed" to "[...] until it returns false". Is that expected?
There was a problem hiding this comment.
absolutely not!
| List.fold_left (fun nothing_to_do atom -> | |
| nothing_to_do && force_remove atom) | |
| true not_installed | |
| List.fold_left (fun nothing_to_do atom -> | |
| force_remove atom && nothing_to_do) | |
| true not_installed |
There was a problem hiding this comment.
I believe this can be simplified into:
| List.fold_left (fun nothing_to_do atom -> | |
| nothing_to_do && force_remove atom) | |
| true not_installed | |
| List.for_all force_remove not_installed |
There was a problem hiding this comment.
Wouldn't for_all have the same issue than the first implem ?
There was a problem hiding this comment.
oops yes, my bad. Please use your suggestion instead
rjbou
changed the title
opam remove --force no longer run commands in current directory
rjbou
added
the
PR: QUEUED
| if not_installed = [] then true else | ||
| if force then |
There was a problem hiding this comment.
| if not_installed = [] then true else | |
| if force then | |
| if not_installed = [] then | |
| true | |
| else if force then |
src/client/opamClient.ml
Outdated
| List.fold_left (fun nothing_to_do atom -> | ||
| nothing_to_do && force_remove atom) | ||
| true not_installed |
There was a problem hiding this comment.
oops yes, my bad. Please use your suggestion instead
| OpamFilename.rmdir d; | ||
| OpamFilename.mkdir d; |
There was a problem hiding this comment.
| OpamFilename.rmdir d; | |
| OpamFilename.mkdir d; | |
| OpamFilename.cleandir d; |
| OpamFilename.rmdir d; | ||
| OpamFilename.mkdir d; |
There was a problem hiding this comment.
I think it would be nice to have a dedicated function in OpamFilename that does that. It doesn't have to be optimized (only remove the directory content but not the directory if it exists already), but having it there simplifies things for whenever we will optimize it.
We have cleandir already, maybe cleandir_create or something like that
There was a problem hiding this comment.
Actually ignore what i just said, i think i would prefer to have this logic deeper in the code anyway. I don't think this should be done in OpamClient (see dae8e6b). This way we avoid useless checks that were already done and we ensure the same logic applies everywhere the OpamAction function is used
This is not ideal as it launches the removal of uninstalled packages before the resolving/apply of the request. But integrating those removal in the solution is not easy as it rely on installed state of package (not installed -> no op).
opam remove --forcelaunches remove commands in current directory #6570sed-cmd#6675