Skip to content

Conversation

@g-arjones
Copy link
Contributor

@g-arjones g-arjones requested a review from doudou August 6, 2025 00:55
@g-arjones g-arjones self-assigned this Aug 6, 2025
@g-arjones g-arjones force-pushed the bundler_2.7_compatibility branch from 637482a to 7c5221a Compare August 6, 2025 16:33
@g-arjones
Copy link
Contributor Author

g-arjones commented Aug 6, 2025

Alternatively, we could also have something like:

def write_bundle_shim(shims_path, bundler_version: nil)
    script = <<~INSTALLER
        require 'rubygems/installer'
        spec = Gem::Specification.find_by_name("bundler", '#{bundler_version || '>= 0'}')
        installer = Gem::Installer.for_spec(spec)
        installer.generate_bin_script("bundle", '#{shims_path}')
    INSTALLER

    pid = fork { exec(env_for_child, Gem.ruby, "-e", script) }
    Process.wait(pid)

    raise "Could not generate 'bundle' binstub" unless $CHILD_STATUS.success?
end

Instead of hardcoding the contents of the shim... @doudou thoughts?

@g-arjones
Copy link
Contributor Author

Tagging @dbcesar since you mentioned you would like to have a look

@doudou doudou force-pushed the bundler_2.7_compatibility branch from 7c5221a to 4405570 Compare August 12, 2025 00:09
@doudou
Copy link
Member

doudou commented Aug 12, 2025

Just did a rebase on master.

@doudou
Copy link
Member

doudou commented Aug 12, 2025

Alternatively, we could also have something like:

def write_bundle_shim(shims_path, bundler_version: nil)
    script = <<~INSTALLER
        require 'rubygems/installer'
        spec = Gem::Specification.find_by_name("bundler", '#{bundler_version || '>= 0'}')
        installer = Gem::Installer.for_spec(spec)
        installer.generate_bin_script("bundle", '#{shims_path}')
    INSTALLER

    pid = fork { exec(env_for_child, Gem.ruby, "-e", script) }
    Process.wait(pid)

    raise "Could not generate 'bundle' binstub" unless $CHILD_STATUS.success?
end

Instead of hardcoding the contents of the shim... @doudou thoughts?

It feels better.

The fork/exec/wait is done by system, which returns false if the command failed.

@doudou
Copy link
Member

doudou commented Aug 12, 2025

Hey ... I might be a tiny bit lost here.

bundle is not generating a binstub for bundle, but gem is, and we install bundler with gem. I don't think this is the right fix, actually. I guess we should be picking bundle from $GEM_HOME/bin instead of from our binstub path.

@g-arjones
Copy link
Contributor Author

I guess we should be picking bundle from $GEM_HOME/bin instead of from our binstub path.

Yes, my initial plan was to store the path to the gem-generated stub in a bundle_executable config setting and then use it instead of the autoproj-generated one in BundlerManager. However, in the end, I figured the current patch was better with regard to backward compatibility.

In hindsight, maybe we could have a "minimal" shim that just exec's the stub generated by gem. Similar to what we do with .autoproj/bin/ruby. This would be backward-compatible and avoid having to write the stub ourselves. What do you think?

@doudou
Copy link
Member

doudou commented Aug 12, 2025

I guess we should be picking bundle from $GEM_HOME/bin instead of from our binstub path.

Yes, my initial plan was to store the path to the gem-generated stub in a bundle_executable config setting and then use it instead of the autoproj-generated one in BundlerManager. However, in the end, I figured the current patch was better with regard to backward compatibility.

In hindsight, maybe we could have a "minimal" shim that just exec's the stub generated by gem. Similar to what we do with .autoproj/bin/ruby. This would be backward-compatible and avoid having to write the stub ourselves. What do you think?

Sounds good. The bundler/rubygem's people plan is definitely to have people use the gem-generated stub, better follow their plan ;-)

@g-arjones g-arjones force-pushed the bundler_2.7_compatibility branch from 4405570 to df831c1 Compare August 12, 2025 19:14
@g-arjones
Copy link
Contributor Author

@doudou Done!

@doudou doudou merged commit 1cd0ffc into master Aug 12, 2025
6 checks passed
@doudou doudou deleted the bundler_2.7_compatibility branch August 12, 2025 20:01
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.

3 participants