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

pre/post-session-commands ignored in switch config #4472

Closed
LasseBlaauwbroek opened this issue Dec 11, 2020 · 9 comments · Fixed by #4476
Closed

pre/post-session-commands ignored in switch config #4472

LasseBlaauwbroek opened this issue Dec 11, 2020 · 9 comments · Fixed by #4476

Comments

@LasseBlaauwbroek
Copy link
Contributor

In Opam 2.0.7, adding a pre/post-session-commands command to a switch-config file has no effect (it does in the global config, however). For example, adding

pre-session-commands: [["echo" "test"]]

Does not display test during installs, even when run in --verbose mode.

@rjbou rjbou added this to To do in Opam 2.1.x via automation Dec 15, 2020
@rjbou
Copy link
Collaborator

rjbou commented Dec 15, 2020

In fact pre/post session command are just not called. Thanks for the report!

@LasseBlaauwbroek
Copy link
Contributor Author

As an additional comment: It would be useful if these commands could be executed before/after the lock on the config file is acquired/released (this is currently not the behavior in the global config). That would allow for additional Opam commands/checks to be run.

@LasseBlaauwbroek
Copy link
Contributor Author

And thanks for the fix!

@rjbou rjbou moved this from To do to In Progress in Opam 2.1.x Dec 16, 2020
@rjbou
Copy link
Collaborator

rjbou commented Dec 16, 2020

It is not possible to lunch them before/after the lock. These lock are acquired on global/switch state load (ro/rw/none according the kind of action needed), usually at the beginning of an opam command. Pre/post session hooks are meant to be launched around sequence of actions (cf. manual), not at each command. Plus they can be conditioned by filter and embed/use some opam variables (installed, new, removed, etc.), those variables are resolved at a quite advanced moment, long after states are loaded.

@rjbou rjbou modified the milestones: 2.1.0~beta4, 2.0.8 Dec 16, 2020
@rjbou rjbou added this to To Do in Opam 2.0.x via automation Dec 16, 2020
@LasseBlaauwbroek
Copy link
Contributor Author

I was thinking that it might be possible to (1) acquire a lock, (2) evaluate the filters, (3) release the lock, (4) execute the pre session hooks (5) re-acquire the lock (6) run the main command sequence. But I suppose there is an opportunity for another opam command to obtain the lock in the meantime and screw things up.

In the ideal situation, it should be possible to somehow lend the current lock to the pre/post session commands. But that would probably be quite difficult to realize...

(This is not really an active feature request, but just some idle speculation.)

@rjbou
Copy link
Collaborator

rjbou commented Dec 16, 2020

yes, as you saw it, losing the lock could result on inconsistent state. Between (4) & (5) another process can change the switch, resulting on a need to recompute set of action, as the old one became invalid. And a new set of action, means reverting the presession command launched first and relaunching it with the new actions, that gives us a new (4) state, etc.

Why do you need a lock free opam at pre/post sessions ?

Opam 2.1.x automation moved this from In Progress to Done Dec 16, 2020
Opam 2.0.x automation moved this from To Do to Done Dec 16, 2020
@LasseBlaauwbroek
Copy link
Contributor Author

LasseBlaauwbroek commented Dec 16, 2020

I'm the developer of Tactician, a machine learning plugin for Coq. I'm injecting some instrumentation code into the builds of Coq packages to learn from their proofs. This is actually working quite well now, but when my package is removed all of the instrumented packages should be recompiled again to reflect this. I'm trying to automate this.

I know I should use (optional) dependencies for that, but that is not possible for my usecase. I would need to be able to declare an optional dependency from the dependency package and not the dependee for that to work, and it would have to be possible to enable/disable this dependency dynamically. I actually also tried adding a dependency dynamically to packages using opam pin edit and that kind of works, but then the packages can no longer be upgraded...

(Note that I know this is a bit of an extreme thing to do, and as such, I don't really expect you guys to provide support for this.)

@rjbou
Copy link
Collaborator

rjbou commented Dec 16, 2020

Oh yup! We already talked on discuss :) I didn't recognized the username.
I see what you want to do. The only way (safe) I see is to print a message to user on removal with the good command, like

You removed tactitian, you need to reinstall some package:
  opam reinstall <all  packages instrumented by tactitian>

But after checking, post-messages field is only on install action, not removal. And messages is printed only at solution time.

In fact, there is a file in opam internals that permit to mark package to reinstall in opamswitchprefix/.opam-init/reinstall, but you can't populate it at removal time as it can be overwritten by the already loaded switch state.

One solution could be to add a removal target to tactician itself, that launches

opam remove tactitian
opam reinstall <all packages...>

but in some distro it can not work as the binary can't be erased if in use...

@LasseBlaauwbroek
Copy link
Contributor Author

LasseBlaauwbroek commented Dec 16, 2020

Ah, I didn't recognize you either :-)

The only way (safe) I see is to print a message to user on removal with the good command

The problem with that is, that there no printing allowed during removal, and there is no post-remove variant of post-install (as far as I know; edit: I now realize that you already said that). I see that #4382 is a change that allows printing during pre/post session commands, but that will only land in 2.08.

One solution could be to add a removal target to tactician itself, that launches

Yes, I can indeed try that. I was thinking of combining that with a pre-session-command that checks if the current session will remove tactician, and just exits with an error if that is the case, giving a message that a cleanup script first needs to be called. My guess is that an error in a pre-session-command will abort the rest of the action? If not it probably should :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants