-
Notifications
You must be signed in to change notification settings - Fork 418
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
v1.2.0 release notes do not mention that RubyThreadLocalVar class was removed #986
Comments
Any class starting with a Ruby, JRuby, Java, TruffleRuby prefix is as fairly obvious an implementation detail. Were you using it? Why instead of using the supported The list of public classes is in the documentation: https://ruby-concurrency.github.io/concurrent-ruby |
@eregon - Nice, thanks for addressing this issue. I don't think that our team (initially) recognized the Thanks again for your quick reply. I appreciate it! |
Hi just wanted to add that Puppet was relying on |
I can't find an issue about that, would have been nice to file one before relying on a library's internals.
|
I agree with @eregon, however I think it's even more clear cut than "any class starting with a Ruby implementation name is an implementation detail". Specifically: Mentions that the class is private and an implementation detail. If you search the documentation, AFAICT, it doesn't even show up: https://www.rubydoc.info/gems/concurrent-ruby/1.1.10 - this applies to all code which is an implementation detail. I think it's fair to say, the authors and maintainers of the library have done their best to clearly mark private implementation details. However, I'm left wondering how code has come to depend on We should consider using @laser do you think we should add the removal and changes to implementation details? I'm not against it, but it might detract from the actual list of public interface changes. Regarding Puppet using an internal implementation detail, well, I can understand the pain, but we are stuck between a rock and a hard place. That interface was never expected to be used publicly. As maintainers, if we don't know about it, we can't do anything about it. If no issue was filed about the leak, or promoting that interface to a public one, I don't see how this situation could have been avoided (I believe I see three potential solutions going forward - the one provided by @eregon makes the most sense to me and is the most future looking. The other two options are:
|
I agree this is a Puppet bug and we're taking steps to remedy. I don't remember the exact specifics of what the underlying issue was, but it doesn't help us now.
Yes that's true, but due to Hyrum's law combined with the fact that it's hard to hide implementation details in Ruby, I would have rather seen this change released as version 2.0. I tend to take a conservative approach because I've been bitten by this kind of thing so many times.
That would be awesome and greatly appreciated. |
FWIW this API was clearly marked internal since 2015 or earlier:
So I don't believe there could be any confusion there whether this was internal or not. |
Hi folks, My two cents, since I'm subscribed to the thread: I think that it was a mistake for our developer to use I also agree that distinction between public and private in Ruby is hard to enforce. I'm not sure how to solve that problem. Based on what I've learned in this thread, I no longer think that the v1.2.0 release notes should have mentioned |
- Bump PORTREVISION for package change Use ThreadLocalVar instead of RubyThreadLocalVar which was removed in 1.2.0. RubyThreadLocalVar is a private class and an implementation detail. Reference: ruby-concurrency/concurrent-ruby#988 ruby-concurrency/concurrent-ruby#986 puppetlabs/puppet@9182bc3
- Bump PORTREVISION for package change Use ThreadLocalVar instead of RubyThreadLocalVar which was removed in 1.2.0. RubyThreadLocalVar is a private class and an implementation detail. Reference: ruby-concurrency/concurrent-ruby#988 ruby-concurrency/concurrent-ruby#986 puppetlabs/puppet@9182bc3
Puppet used to rely on an internal API that was removed. See the follwing issue for details: ruby-concurrency/concurrent-ruby#986
* update rails gems * Upgrade puppet to fix regression. Puppet used to rely on an internal API that was removed. See the follwing issue for details: ruby-concurrency/concurrent-ruby#986 --------- Co-authored-by: David Roetzel <david@roetzel.de>
Problem
The v1.1.0 release defined a class called
RubyThreadLocalVar
(link). That class does not exist as of v1.2.0 (link) - the file in which it was defined was removed in commit 30465396 - but the removal was not mentioned in the v1.2.0 release notes. I'm not sure if this was an oversight or a bug.Proposed Solution
Either the changelog should be updated to reflect the breaking change, or the
RubyThreadLocalVar
should be restored.The text was updated successfully, but these errors were encountered: