Allow post-install-commands hooks to modify/remove installed files

[lefessan]

This modification is necessary because opam-bin uses the post-install-commands to share files between switches using hard links. Unfortunately, without this PR, such actions modify the digests of the files just after their recording, so that opam remove will complain that the installed files have been modified.

Please update master_changes.md file with your changes.

[AltGr]

LGTM, thanks!

[hannesm]

I'm a bit sceptical this is a good idea - is there an overview which phases and hooks are allowed to do what? At least, the documentation should be adjusted:

The post-install-commands hook also has access to an extra variable installed-files which expands to the list of files and directories added or modified during the installation of the package. Note that this hook is run after the scan for installed files is done, so any additional installed files won't be recorded and must be taken care of by a pre-remove-commands hook.

wouldn't such a post-install and a corresponding pre-remove work for opam-bin?

[lefessan]

wouldn't such a post-install and a corresponding pre-remove work for opam-bin?

opam-bin already adds files during the post-install hook that it later removes using the pre-remove hook.
But in this particular case, it is not about adding new files, but changing files that have just been installed. Even if opam-bin records its changes, it won't be able to restore the original files, so opam will keep complaining that the files that were installed were either removed or modified. Keeping a copy of the file would defeat the purpose which is to save space on the disk by sharing installed files between switches.

Note that this PR does not allow a post-install hook to add new files in the changes, only to modify/remove some of them.

[avsm]

Lets not be hasty about adding in hook features this late in opam 2.1's release cycle without careful justification, please. I think it's important to understand the power of these hooks:

• does this hook facility bypass the sandboxing process?
• can a post-install hook modify files installed by other packages, or just those under its original control?

[lefessan]

Lets not be hasty about adding in hook features this late in opam 2.1's release cycle without careful justification, please. I think it's important to understand the power of these hooks:

• does this hook facility bypass the sandboxing process?
• can a post-install hook modify files installed by other packages, or just those under its original control?

Hooks completely bypass the sandboxing process : it is already possible for post-install hooks (and other hooks) to modify files anywhere on the system.

[AltGr]

Just clarifying two points:

• sandboxing is itself a hook, so command-wrapping hooks can be put inside or outside the sandbox by customising the order of definition in ~/.opam/config; pre/post hooks are not sandboxed.
• this PR really doesn't allow hooks to do anything they couldn't: it just actually handles the case where the post-install hook modifies files in a more consistent way.

The latter explains I have no objection, since the PR does not change existing features but improves handling for a specific case of an existing feature.

EDIT: but yes the PR title is pretty misleading :)

[hannesm]

thanks for the explanation @AltGr.

[avsm]

Thanks for the explanations. Could we get the Changes entry to be more accurate then? Specifically, it appears that opam 2.1 is now tracking the results of ~/.opam changes made during the post-install hook execution and registering those with opam. Are there any other implications of making this change (e.g. to opam list --show-files?)

[lefessan]

Thanks for the explanations. Could we get the Changes entry to be more accurate then? Specifically, it appears that opam 2.1 is now tracking the results of ~/.opam changes made during the post-install hook execution and registering those with opam. Are there any other implications of making this change (e.g. to opam list --show-files?)

There is already an entry in the Changes that is added with the PR. Note that only the files that were installed by the package are checked for modification, other files added/modified by the post-install hook are not, and should be removed/restored by the plugin using the pre-remove hook.

[lefessan]

Are there any other implications of making this change (e.g. to opam list --show-files?)

Users that do not use opam-bin usually have no post-install hook, so nothing is changed for them (no additional cost). For those using a post-install hook, the PR does not increase the power of what the hook can do, it just prevents them from corrupting opam changes files.

[AltGr]

Just one last tidbit, the last sentence in the manual here should be updated to reflect the change. The doc doesn't become incorrect, but just precise something along the lines of "Modified installed files will be handled correctly by opam"

Source: https://github.com/ocaml/opam/blob/master/doc/pages/Manual.md

[lefessan]

Just one last tidbit, the last sentence in the manual here should be updated to reflect the change. The doc doesn't become incorrect, but just precise something along the lines of "Modified installed files will be handled correctly by opam"

Source: https://github.com/ocaml/opam/blob/master/doc/pages/Manual.md

Indeed, I have modified the Manual file in this new version.

[rjbou]

Small changes, but LGTM!
In order to keep consistency with function inf OpamDirTrack, update should take a dirname and not a string.