-
Notifications
You must be signed in to change notification settings - Fork 185
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
Superfluous Object:: in module name #3683
Comments
Hey! Just so that you know :), I just published Zeitwerk 2.7 with that change. Due to this issue, Zeitwerk 2.7 does not work with TruffleRuby right now. |
Could you point me to something that breaks?
works for example, so while this is still a bug in TruffleRuby, |
I think it would be nice to workaround this in Zeitwerk if possible, so that the latest TruffleRuby release is not broken. |
"Object::M is a valid constant path to reference M." yeah, the problem is Zeitwerk keep the expected name in a Hash and need to look it up. This can be worked around for sure, but it's a pretty hot loop, so should be done with care (or scoped under |
People can just require |
Could you point me to the code doing that? Maybe the key could be the module itself instead of strings?
I suppose the error won't be clear so it's still a very poor user experience. I just tried |
No, the whole point is that this is registered before the module is loaded. |
Because the error only shows up with namespaces defined in files. Their child constants won't be found.
There are projects where classes/modules are not hashable, learned that the hard way time ago. |
The constant already exists by the time
How is that possible? All |
They had In any case, this is a hot code path and the implementation was chosen among several options to be the most performant one. I'd need nested hashes to do this (because the key is a pair (mod, cname) conceptually). I don't plan to revisit this implementation any time soon, unless I find something even more performant and backwards compatible. |
@eregon, can you help me understand this thread? TruffleRuby has a bug. I report it in advance. The reaction I would expect would be "thanks, we'll ship a fix as soon as possible". But I am feeling instead kind of a push back, and a proposal that consists on investigating in which way TruffleRuby is incompatible and that I release a new version to workaround the fact that TruffleRuby won't ship a new version. Or that I change an implementation that is correct. Please don't get me wrong, I am asking this with sincerity. Then, I see this as marked to be released in March 2025 (!). Why? |
First of all, thank you for reporting the bug, we forgot to say that. @andrykonchin made a fix today in #3688 and I reviewed, it should be good to go. #3683 (comment) worried me because it sounds like you released a new Zeitwerk version without caring about this issue. It's fair enough since you filed this issue and TruffleRuby is not currently in Zeitwerk's CI (due to #2431 which I tried to fix but I got endless complications and CI failures due to the complexity of CRuby autoload semantics and never got to merge it, it's still an open PR, in hindsight it could have been nice to only skip that flaky test but keep running the other tests).
That is simply the next milestone & next TruffleRuby feature release (same schedule as GraalVM for releases). From https://www.graalvm.org/release-calendar/#graal-languages it's too late for 24.1.1 (last fix was Oct 1). I was asking these questions because indeed I was hoping for a quick workaround or fix in Zeitwerk, which could be released much sooner than TruffleRuby. Caching on the Module instead of its name feels intuitively better but of course you make good points that it would need nested hashes or |
I cared, and I reported it in advance. But got no feedback. If you guys had been like, "oh, let us work on this one, can you hold releasing a bit?" I would have certainly be glad to do so. But the ticket was silent and TruffleRuby is the project interested in this. The ticket is not "could you please fix this for Zeitwerk?". The ticket is, "you probably want to know about this (and BTW the next release of Zeitwerk won't fully work without this fixed, but that is up to you guys)". On the other hand, Zeitwerk 2.7 has been in the oven for some time, and it is a project milestone. So I shipped. And I optimistically thought TruffleRuby would eventually address this one.
Yes, that.
Wow, I did not know that. That makes me think about Rails 8, I planned to make it depend on Zeitwerk 2.7. |
I'll try to make a PR to Zeitwerk to see what a workaround looks like (should be pretty trivial, like a |
Thanks @eregon. I'll also look for spots in which I can leverage comparing by identity, maybe we can save computing a few permanent names. |
* See oracle/truffleruby#3683 * Without this the test suite has 6 failures, 26 errors.
* See oracle/truffleruby#3683 * Without this the test suite has 6 failures, 26 errors.
I have also seen that class Module
def const_added(cname)
p const_get(cname).name
end
end
H = Class.new prints |
I think that's the same issue (will verify by adding a spec for that case), it's also because of the order in https://github.com/oracle/truffleruby/pull/3688/files#diff-c60ad35a6a0531d8ba253c54bce6ce93009fd357fee591615137609f1929c5ab where it used to be "set constant, call const_added, set full name" where the PR fixes it to "set full name, set constant, call const_added". EDIT: |
Interesting, why isn't the name |
Because
#3688 fixes that case though should it happen before the full name is set. |
Fixed in 495a0e5 |
* We need to use truffleruby-head for now because of oracle/truffleruby#3683
For the record and to document this, this issue causes
The easiest solution is to use truffleruby-head and truffleruby 24.1.2+ when it is released (21 Jan 2025). Using a zeitwerk < 2.7 also works as documented here, by modifying the Gemfile after One can also generate the application on CRuby and then run it on TruffleRuby. |
Just for tracking sake, it seems that even with truffleruby 24.2.0 (dev), we are still facing some issues with I am getting an Internal Serve Error
The workaround gem "zeitwerk", "< 2.7" is still working |
Cannot reproduce the issue mentioned above with Rails 8.0.0.rc1 and Rails 8.0.0.rc1:
Rails 7.2.1.1:
|
Did you click http://127.0.0.1:3000? The exception is thrown when you click to the above URL. |
Seems related. The test suite of Zeitwerk passes with I have also added a workflow to run the test suite against TruffleRuby HEAD daily. I have a similar one for CRuby HEAD. |
If you want a faster way to reproduce, this should suffice:
should trigger the error. The key point is that |
Right, thank you for clarification.
Thank you! I've checked again (on 44ded82) and don't reproduce the issue:
@bachos Is the issue reproducible in your environment on the TruffleRuby master? |
I got |
Looks like truffleruby-jvm-24.2.0-ea.18-linux-amd64.tar.gz was made available but it is based 0e05bfe. 😿 |
Could you try
|
It is working, thanks a lot. |
Hi! I am replacing
TracePoint
withModule#const_added
in Zeitwerk, and noticed the new test suite does not pass on TruffleRuby (this work is still not released).The root cause is this:
That prints "Object::M", instead of "M".
I suspect it is related to internal callback logic specifically, because
M.name
is "M" later.Version is
/cc @byroot
The text was updated successfully, but these errors were encountered: