Add hooks in quarks

[capital-G]

Purpose and Motivation

Implements #5753
Before writing docs and tests I want to make sure that this implementation is fine.

I switched form upgrade->update and remove->uninstall terminology in order to reflect more the terminology used within SuperCollider than in pacman.

Types of changes

• Documentation
• New feature

To-do list

• Code is tested
• All tests are passing
• Updated documentation
• This PR is ready for review

[scztt]

Consider wrapping the hook callback in a try with a more specific error message? I don't think we can totally protect or clean-up in case of an error while calling a hook, but it would be good to report errors as something more than a random sclang exception (these are hard enough interpret for people....). I'm imagining something like this would be enough:

try {
  this.data[hook].value();
} {
   |e|
   "Encountered error while running hook '%' for Quark('%'). Installation or update for this Quark may not have completed correctly.".warn;
   e.throw(); // re-throw
}

[capital-G]

Thanks for the feedback @scztt - do you think the additional commits are sufficient? if so, I'll start to write docs for it :)

[capital-G]

Updated docs

[capital-G]

Added tests, but not all tests are passing, e.g.

FAIL: a TestCoreUGens: test_ugen_generator_equivalences - "Duty.ar(SampleDur.ir, 0, x) == x"
1 of 48000 items in array failed to match. Displaying arrays from index of first failure (0) onwards:
FloatArray[ 0.60310339927673, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0....etc...
! = 
0

but it seems this was not broken by this PR

[capital-G]

Anything thats missing from a merge?

[joshpar]

@scztt - how are you feeling with the changes?

[capital-G]

Should this be considered for https://github.com/supercollider/supercollider/milestone/24 ? Makes sense to me to include it in an major version upgrade, making it easy for checking if this hook functionality is existing.

[dyfer]

@scztt just checking in, would you be able to follow up on your review?

[telephon]

@capital-G I saw that scott had some comments in the review – have you checked them?

[capital-G]

Bump

[telephon]

@capital-G

This COULD be a problem, sadly... git.checkout here can change the Quark data, and this potentially change the hooks. It MIGHT be that what we actually want to do here is:

1. Run `\preUpdate`

2. `git.pull()` / `git.checkout`

3. Reload `data`

4. run `\postUpdate`

It would probably have to be clearly documented that \preUpdate is always the OLD hook, and \postUpdate is always the NEW hook.

Without doing this, we'd only ever be running the old post hook, which seems wrong.

[capital-G]

Yes, I documented this behavior, see this comment #5780 (comment)

[telephon]

looks good to me now.

[dyfer]

Thanks for the PR @capital-G !

I wanted to merge this but I noticed that the tests did not run in the CI (that sometimes happens) so I've run the tests locally after building from this branch and there were some failures. Could you please check and fix the tests?

RUNNING UNIT TEST 'TestQuark'
-> TestQuark
PASS: a TestQuark: test_parseQuarkFileHooksAsFunction - Hook 'preInstall' should be parsed as function
PASS: a TestQuark: test_parseQuarkFileHooksAsFunction - Hook 'postInstall' should be parsed as function
PASS: a TestQuark: test_parseQuarkFileHooksAsFunction - Hook 'preUpdate' should be parsed as function
PASS: a TestQuark: test_parseQuarkFileHooksAsFunction - Hook 'postUpdate' should be parsed as function
PASS: a TestQuark: test_parseQuarkFileHooksAsFunction - Hook 'preUninstall' should be parsed as function
PASS: a TestQuark: test_parseQuarkFileHooksAsFunction - Hook 'postUninstall' should be parsed as function
Installing TestQuark
Run preInstall hook
Run postInstall hook
TestQuark installed

FAIL: a TestQuark: test_runQuarkInstallHooks - Check if hook 'preInstall' was called


FAIL: a TestQuark: test_runQuarkInstallHooks - Check if hook 'postInstall' was called

PASS: a TestQuark: test_parseQuarkFileData - Quark name should be parsed from quark file
PASS: a TestQuark: test_parseQuarkFileData - Quark version should be parsed from quark file

UNIT TESTS FOR 'TestQuark' COMPLETED
There were failures:
a TestQuark: test_runQuarkInstallHooks - Check if hook 'preInstall' was called
a TestQuark: test_runQuarkInstallHooks - Check if hook 'postInstall' was called

[dyfer]

BTW we are planning to do a release candidate for 3.13 this weekend. I'll add this to the milestone so that it doesn't fall off the radar.

[telephon]

@dyfer seems like the CI has completed, so you can resolve that request.

[dyfer]

@telephon
The test suite job in GHA took 1 minute to finish, which unfortunately means that it exited (successfully...) without actually running the tests. I've seen that happening before. I'm sorry this is so unreliable, but I was unsuccessful in making it better so far.

Because of this I have built this PR locally and noticed that some tests fail. If I missed something, please let me know.

[capital-G]

The tests were broken when I created the branch - should i merge or rebase (what is better in this scenario?) the main branch, this should trigger the test suite with the proper changes right - if it works its fine, if it fails than it shouldnt be merged.

[dyfer]

@capital-G Please take a look at my message above. I've only run tests added in this PR (TestQuark) and found failures there. Other failures of course are unrelated and would not block merging this PR. Can you please check the tests added here? If they only fail on my machine but not on yours, then we'd need to investigate further.

[capital-G]

Oh, I oversaw the details section, thanks for the hint, will fix this.

[telephon]

@capital-G why did you close this? I think the tests probably just make some wrong assumption. If you like, we can look through it.

[capital-G]

See bottom of #5487 (comment)
If someone else wants to continue/finish this please do so, the branch is still existing.

[telephon]

BTW we are planning to do a release candidate for 3.13 this weekend. I'll add this to the milestone so that it doesn't fall off the radar.

@dyfer depening on the schedule, maybe you could add #5907 to the milestone for 3.13 ?

[dyfer]

@dyfer depening on the schedule, maybe you could add #5907 to the milestone for 3.13 ?

@telephon yes, I was planning to do that. Since we just did 3.13.0-rc1, I'm trying to decide whether we should make 3.13.0-rc2 with that, or should that PR wait until 3.13.1. Technically I think new functionality shouldn't be added from one rc to the next, but this is relatively small and I was hoping it would be included... So, I'm planning for #5907 to go into 3.13.1 at the latest, but possibly earlier.