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

Stop bundler eagerly loading all specs with exts #6945

Merged

Conversation

segiddins
Copy link
Member

We were setting the wrong extension_dir for git specs stubs

Additionally, the call to self.extension_dir was loading the remote spec, which was avoidable since the stub had an extension dir (and in fact its #gem_build_complete_path does exactly what we want anyway)

Finally, now set the base_dir when loading the remote_spec from a stub specification, since the git source sets the base dir for stubs based on where the spec will be installed to, and we want to preserve that so the base_dir for the loaded spec & the stub are the same

What was the end-user or developer problem that led to this PR?

Bundler was eagerly loading all gemspecs that had extensions in their stubs

What is your fix for the problem, implemented in this PR?

Avoid calling methods that force loading the remote spec inside of #missing_extensions?

Make sure the following tasks are checked

We were setting the wrong `extension_dir` for git specs stubs

Additionally, the call to `self.extension_dir` was loading the
remote spec, which was avoidable since the stub had an extension dir
(and in fact its #gem_build_complete_path does exactly what we want
anyway)

Finally, now set the base_dir when loading the remote_spec from a
stub specification, since the git source sets the base dir for stubs
based on where the spec _will_ be installed to, and we want to preserve
that so the base_dir for the loaded spec & the stub are the same
@segiddins segiddins force-pushed the segiddins/avoiding-eager-loading-specs-with-extensions branch from e1833cf to a94acb4 Compare September 12, 2023 19:27
@segiddins segiddins merged commit 1549d5e into master Sep 21, 2023
92 checks passed
@segiddins segiddins deleted the segiddins/avoiding-eager-loading-specs-with-extensions branch September 21, 2023 18:28
deivid-rodriguez pushed a commit that referenced this pull request Oct 13, 2023
…ng-specs-with-extensions

Stop bundler eagerly loading all specs with exts

(cherry picked from commit 1549d5e)
deivid-rodriguez pushed a commit that referenced this pull request Oct 13, 2023
…ng-specs-with-extensions

Stop bundler eagerly loading all specs with exts

(cherry picked from commit 1549d5e)
deivid-rodriguez pushed a commit that referenced this pull request Oct 13, 2023
…ng-specs-with-extensions

Stop bundler eagerly loading all specs with exts

(cherry picked from commit 1549d5e)
deivid-rodriguez pushed a commit that referenced this pull request Oct 16, 2023
…ng-specs-with-extensions

Stop bundler eagerly loading all specs with exts

(cherry picked from commit 1549d5e)
@hanyang-tony
Copy link
Contributor

For universal rubies, the change to def gem_build_complete_path is incorrect. Previously, extension_dir is invoked on the Bundler::StubSpecification, which is a child class of Specification. After the change to stub.gem_build_complete_path, @Stub is actually an instance of Gem::StubSpecification, thus gem_build_complete_path is from Gem::BasicSpecification.

The problem arise when in bundler, we replace the 'universal' part of Gem::Platform with the arch. But we still use the universal name for Gem::Specification, this is defined in 'rubygems_ext.rb'. Now the stub is using extension_dir defined in Gem::BasicSpecification, so the 'universal' part ended up being swapped by arch.

So, we install native extensions to 'extensions/universal-darwin-22/', but finding them in 'extensions/x86_64-darwin-22/'. Leading to native extension gems can never be installed on universal ruby.

hanyang-tony added a commit to hanyang-tony/rubygems that referenced this pull request Oct 19, 2023
Since rubygems#6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.
hanyang-tony added a commit to hanyang-tony/rubygems that referenced this pull request Oct 19, 2023
Since rubygems#6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.
hanyang-tony added a commit to hanyang-tony/rubygems that referenced this pull request Oct 19, 2023
Since rubygems#6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.
hanyang-tony added a commit to hanyang-tony/rubygems that referenced this pull request Oct 20, 2023
Since rubygems#6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.
hanyang-tony added a commit to hanyang-tony/rubygems that referenced this pull request Oct 23, 2023
Since rubygems#6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.
hanyang-tony added a commit to hanyang-tony/rubygems that referenced this pull request Oct 24, 2023
Since rubygems#6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.
hanyang-tony added a commit to hanyang-tony/rubygems that referenced this pull request Oct 27, 2023
Since rubygems#6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.
hanyang-tony added a commit to hanyang-tony/rubygems that referenced this pull request Oct 30, 2023
Since rubygems#6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.
deivid-rodriguez pushed a commit to hanyang-tony/rubygems that referenced this pull request Oct 31, 2023
Since rubygems#6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.
hanyang-tony added a commit to hanyang-tony/rubygems that referenced this pull request Nov 1, 2023
Since rubygems#6945 the extension dir changed to Gem::BasicSpecification's implementation, we didn't hook that in rubygems_ext.rb. So for universal rubies, we ended up using the universal platform name when installing, but arch replaced platform name when checking. This lead to native extensions can never be correctly installed on universal rubies.

Hook Gem::BasicSpecifications so the behavior is consistent on installing and checking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants