Skip to content
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

Add workaround for truffleruby 24.1.0 and add truffleruby in CI #303

Closed
wants to merge 5 commits into from

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Oct 12, 2024

It turns out after the workaround for oracle/truffleruby#3683 there were only 2 failures in the test suite.
One is well known: oracle/truffleruby#2431.
The other I didn't find out why after a quick look. Given it's the only failure left it seems best to skip it for now and still add truffleruby in CI to notice any potential issue.
It would be good to investigate that failure but I don't have time for that now.
I did some quick checks and const_added is called correctly for things like Hotel = Class.new so that doesn't seem the problem.

* The fact it's a Symbol is key, otherwise `cname.name` would be NoMethodError.
* See oracle/truffleruby#3683
* Without this the test suite has 6 failures, 26 errors.
@eregon
Copy link
Contributor Author

eregon commented Oct 12, 2024

@fxn
Copy link
Owner

fxn commented Oct 12, 2024

I explained the root cause in oracle/truffleruby#3683:

class Module
  def const_added(cname)
    puts const_get(cname).name
  end
end

module M
end

which prints "Object::M" instead of "M". As you know, this is not what it is supposed to print, because from the user point of view, M stores a module object, the assigment already happened, and therefore the module object should have a permanent name, and it has to be "M".

That root cause manifests in the callback on_namespace_loaded , because we access a hash keyed by namespace name (when the hash entry is created, the namespace is not yet loaded, and it may never be).

If the only Module#name that misbehaves in const_added is that one, I think we should patch const_added and not impact all real_mod_name.

@eregon
Copy link
Contributor Author

eregon commented Oct 12, 2024

OK, I'll try to move the workaround to on_namespace_loaded

@eregon
Copy link
Contributor Author

eregon commented Oct 12, 2024

I tried with eregon@b59c019 but that's not enough https://github.com/eregon/zeitwerk/actions/runs/11305154330/job/31444355057
As you can see with the backtrace there:

/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/cref.rb:34:in `path'
/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/loader.rb:492:in `autoload_file'
/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/loader.rb:449:in `block in define_autoloads_for_dir'
/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/loader/helpers.rb:47:in `block in ls'
/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/loader/helpers.rb:25:in `each'
/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/loader/helpers.rb:25:in `ls'
/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/loader.rb:445:in `define_autoloads_for_dir'
/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/loader/callbacks.rb:85:in `block in on_namespace_loaded'
/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/loader/callbacks.rb:84:in `each'
/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/loader/callbacks.rb:84:in `on_namespace_loaded'
/home/runner/work/zeitwerk/zeitwerk/lib/zeitwerk/core_ext/module.rb:6:in `const_added'

It's called quite deep and using the workaround in multiple places starts to feel brittle.

So I think doing the check for all real_mod_name is more robust and practical.
I'm not too concerned about performance, name.start_with?('Object::') is fairly cheap and I'd think real_mod_name is only called a few times per file, right?
The next truffleruby release will have the behavior fixed and not use this workaround thanks to the version check.

@eregon
Copy link
Contributor Author

eregon commented Oct 12, 2024

For completeness, eregon@36c800f passes the test suite but I'm not confident it's using the workaround in all the needed places, there are many other calls to real_mod_name I haven't reviewed (I only changed the ones shown by the test suite) and it seems non-trivial to prove whether they can be called from inside const_added or not (also considering the code will change).

It's great Zeitwerk has this real_mod_name helper, that already makes the workaround pretty localized, I don't think we need to be more precise than that, but it could always be done later if worth it.

@fxn
Copy link
Owner

fxn commented Oct 12, 2024

Cool, that is what I had in mind in that comment in the original thread, I cannot know which is the exact compatibility difference in TruffleRuby. I spotted one case.

I'm out now on my phone, will look at the details later. Thanks for looking into it @eregon!

@fxn
Copy link
Owner

fxn commented Oct 12, 2024

@eregon could you confirm these working hypothesis I have about the bug? (this is the hard part from my side, understanding the blast radius):

  1. Even if Module#name may be wrong while const_added has not returned, it is always correct after it, even for those class/modules whose name was incorrect. That is, the wrong name is not cached as permanent.
  2. Any other class or module object different from const_get(cname, false) while const_added runs reports a correct name.
  3. This issue only happens with top-level constant. If the name does not start with "Object::", then it is correct.

@fxn
Copy link
Owner

fxn commented Oct 12, 2024

Nah, this is more complicated:

class Module
  def const_added(cname)
    p const_get(cname).name
  end
end

H = Class.new

This prints nil instead of "Object::H" (that is why some tests fail even deleting the prefix).

And this is only what I can find by trial and error.

For me, the bottom line is that the internal state in TruffleRuby is not correct in the current stable release, and I don't want to assume all possible side-effects can be anticipated and corrected with a workaround. I would have considered a trivial one, but it starts to feel brittle and out of the scope of this gem.

So, my resolution is the following:

  1. I won't make Rails 8 require Zeitwerk 2.7. Since 2.7 satisfies the version constraint in railties, new applications will get 2.7 in practice.
  2. TruffleRuby users need to pin Zeitwerk to 2.6, or run an edge version with this fixed.

@fxn fxn closed this Oct 12, 2024
@fxn
Copy link
Owner

fxn commented Oct 12, 2024

I have updated the README to document this (1ea9729).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants