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

Make Rails engine path check more specific #1925

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

vinistock
Copy link
Member

Motivation

We load Rails engines that are defined inside the current gem's directory and extract symbols to process from that engine's eager auto load paths.

When Rails is installed through a release version, this works perfectly because each gem is placed in a sibling directory

~/.gem/ruby/3.3.3/gems/rails-7.1.4
~/.gem/ruby/3.3.3/gems/actionmailbox-7.1.4
~/.gem/ruby/3.3.3/gems/activestorage-7.1.4
...

However, when Rails is installed via a git source, Bundler nests all of the sub-gems inside the same directory

~/.gem/ruby/3.3.0/bundler/gems/rails-e3ea4c74124f/
~/.gem/ruby/3.3.0/bundler/gems/rails-e3ea4c74124f/activestorage
~/.gem/ruby/3.3.0/bundler/gems/rails-e3ea4c74124f/actionmailbox
...

The result is that we get symbols that belong to the sub-gems (activestorage, actionmailbox) placed inside the Rails RBI, which is incorrect. Note that this problem only happens if another gem eagerly requires one of the engines.

Implementation

Currently, we are checking if the engine's root is anywhere inside the gem's full path.

I'm proposing that we changed to be more specific and only check if the engine root is exactly equal to the gem path.

I don't know the original logic was intentional. If we do need to check sub-directories, then I'm not sure how we would go about fixing this. Maybe we can verify if the gem was installed through a git source and only do the more specific matching in that case?

I did verify this change on Core and everything looks fine.

Tests

Added a test that reproduces the behaviour.

@vinistock vinistock self-assigned this Jun 14, 2024
@vinistock vinistock requested a review from a team as a code owner June 14, 2024 19:03
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find, thank you!

I wonder if there are any gems that set up and configure their engines to use a subdirectory of the gem path as the root path, but I doubt that's something we have to address now. If someone complains, we can address it then.

@vinistock vinistock merged commit aaf1d32 into main Jun 14, 2024
32 checks passed
@vinistock vinistock deleted the vs/fix_git_source_eager_loaded_rails_engines branch June 14, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants