supercollider icon indicating copy to clipboard operation
supercollider copied to clipboard

Add hooks in quarks

Open capital-G opened this issue 3 years ago • 8 comments

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

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

capital-G avatar May 20 '22 08:05 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 avatar May 27 '22 22:05 capital-G

Updated docs

grafik

capital-G avatar May 29 '22 13:05 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 avatar May 29 '22 17:05 capital-G

Anything thats missing from a merge?

capital-G avatar Jul 07 '22 09:07 capital-G

@scztt - how are you feeling with the changes?

joshpar avatar Jul 15 '22 17:07 joshpar

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.

capital-G avatar Sep 11 '22 13:09 capital-G

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

dyfer avatar Sep 11 '22 17:09 dyfer

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

telephon avatar Sep 12 '22 06:09 telephon

Bump

capital-G avatar Nov 11 '22 11:11 capital-G

@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.

telephon avatar Nov 11 '22 13:11 telephon

Yes, I documented this behavior, see this comment https://github.com/supercollider/supercollider/pull/5780#issuecomment-1140453715

capital-G avatar Nov 11 '22 14:11 capital-G

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 avatar Nov 11 '22 21:11 dyfer

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

telephon avatar Nov 12 '22 16:11 telephon

@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.

dyfer avatar Nov 12 '22 19:11 dyfer

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.

capital-G avatar Nov 13 '22 02:11 capital-G

@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.

dyfer avatar Nov 13 '22 02:11 dyfer

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

capital-G avatar Nov 13 '22 02:11 capital-G

@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.

telephon avatar Nov 13 '22 18:11 telephon

See bottom of https://github.com/supercollider/supercollider/pull/5487#issuecomment-1312736678 If someone else wants to continue/finish this please do so, the branch is still existing.

capital-G avatar Nov 13 '22 18:11 capital-G

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 ?

telephon avatar Nov 14 '22 08:11 telephon

@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.

dyfer avatar Nov 14 '22 08:11 dyfer