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

Deprecate --default option from install command #7588

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hsbt
Copy link
Member

@hsbt hsbt commented Apr 18, 2024

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

The current --default option is confused for RubyGems users.

This option is special behavior for default gems. But I'm not sure why it only generate executable, not copy library files and other processes.

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

It leads complicated environment and there is no merit for them. I don't allow that users install gem as default gems from core maintainer's view.

Finally, I'll show deprecated message and removed code about this option from rubygems.

Closes #7005.

Make sure the following tasks are checked

@hsbt hsbt force-pushed the deprecate-default-option branch 4 times, most recently from 89277af to 0f34a74 Compare April 18, 2024 09:06
@deivid-rodriguez
Copy link
Member

@hsbt I agree with this approach. The --default option is problematic and causes broken installs. The easiest option would be to remove it and consider default gems fixed. Is this ready for review?

@hsbt
Copy link
Member Author

hsbt commented Apr 25, 2024

👌 It's ready for review now.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Right now this PR is deprecating the flag, but changing the behavior by making the flag a noop and install the gem normally, correct?

I believe this may be acceptable, because I don't see how the current implementation is helpful. However, we currently document exactly what the flag does, so may be potentially useful for someone, and the more conservative approach would be to first deprecate the flag without changing behavior, and then remove everything.

I think the latter option is preferable, but open to do it differently.

)
installer.install
File.delete installer.spec_file
installer.write_default_spec
Copy link
Member

Choose a reason for hiding this comment

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

I think this will install a full copy of bundler in the standard $GEM_HOME. I don't think it's a problem per se, but it makes Bundler inconsistent with other default gems with executables provided with Ruby (for example, irb, which includes a placeholder gem directory with just the executable).

We could try to iterate on this but perhaps for now it's better to not use Gem::Installer at all here and create the files necessary (executables, and gemspec) manually, to keep the exact same folder structure gem update --system creates now.

"Add the gem's full specification to",
"specifications/default and extract only its bin") do |v,_o|
options[:install_as_default] = v
end
Copy link
Member

Choose a reason for hiding this comment

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

Should we use deprecate_option here, to also give a runtime deprecation message? The way it is right now, I think it only appears as deprecated in documentation.

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