search

harlowja / fasteners

A python package that provides useful locks.
Apache License 2.0
243 stars 45 forks source link

a patch release 0.16.1 broke "API" #71

Closed yarikoptic closed 3 years ago

yarikoptic commented 3 years ago

On this wonderful Monday morning I just want to whine and beg to be more careful about changing public (since neither ._protected nor .__private -- makes it public) API surface. 0.15-36-gade69f8 (AKA 0.16.1~4^2) removed .acquired attribute from _InterProcessLock.

0.16.1 got released Fri Jun 4 09:57:28 2021 +0200 and that broke our DataLad where we did use .acquired to assess if lock was acquired: https://github.com/datalad/datalad/blob/HEAD/datalad/support/locking.py#L91 . It might be that our code there is suboptimal and we should have not relied on .acquired but it was a part of public API so we did.

psarka commented 3 years ago

Crap, I forgot to branch off the 0.16 release and released off of master insted. Will check how to fix this.

yarikoptic commented 3 years ago

I think the easiest would be to revert that commit (or any other pertinent), release 0.16.2 (off the branch off or after reverting what should not be there) and kill 0.16.1 on pypi

but in the long run -- .acquired will be removed?

yarikoptic commented 3 years ago

BTW, git tag for 0.16 seems was not pushed to github

psarka commented 3 years ago

Added the missing tag and will release the 0.16.2

@harlowja Could you, please, yank the 0.16.1 release from the pypi? I don't have the sufficient permissions for that.

psarka commented 3 years ago

Released the 0.16.2, @yarikoptic sorry for the disturbance.

As for the .acquired method I have to admit, I'm a bit lost what to do. On one hand it is obviously "incorrect", as by the time you get the result you no longer know if the lock is acquired or not, on the other hand people keep on using it and even POSIX has F_GETLK.

yarikoptic commented 3 years ago

Thank you @psarka !

I wish I could share my "wisdom" on how to proceed about .acquired but I don't know enough of locking business :-/

In our case - we can avoid using it in the code quite easily but tests somehow really "wanted" it. I came up with an ugly workaround for that and we should probably look toward not relying on .acquired, and then I would stop carrying if it goes away . ;-)
But if you are to remove it, it might be worth a deprecation cycle, making access to .acquired (making it a property?) to spit out a deprecation warning that it is going to get removed in e.g. 0.18.0 (if warning is added now or to 0.17) so there is a full ".minor" cycle with that warning an ppl get alerted.

yarikoptic commented 3 years ago

I guess we can consider it fixed by 0.16.2 (FWIW -- tested that it was released, and pip installable, and our tests don't break). Thanks again