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

Behaviour changes with default gems installer #2166

Closed
wants to merge 10 commits into from
Closed

Conversation

hsbt
Copy link
Member

@hsbt hsbt commented Feb 1, 2018

Description:

A current installer of default gems only installs executable file and gemspec. It didn't resolve the real world problem.

Default gems provide to upgrade standard libraries like gem update openssl. But current installer used by gem install openssl --default is inconsistency behavior with gem update.

I change its behavior to install gem files and build native extensions. It can upgrade instructions for the old version of Ruby like 2.3 or 2.4.


Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

@hsbt
Copy link
Member Author

hsbt commented Feb 2, 2018

Related with #677

@@ -112,6 +112,31 @@ class Gem::Specification < Gem::BasicSpecification

VALID_NAME_PATTERN = /\A[a-zA-Z0-9\.\-\_]+\z/ # :nodoc:

DEFAULT_GEMS_LIST = %w[
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this pull from Gem::Specification.select(&:default_gem?).map(&:name) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not working. default gems were determined without ruby version.

Copy link
Member

Choose a reason for hiding this comment

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

default gems were determined without ruby version

I'm not sure I understand what you mean by this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Default gems do not depend on Ruby version.

Ex. We can install default gems from listed by https://stdgems.org/ with old ruby. But is not activated in the current specification. I hope to activate them with old ruby.

Copy link
Member

Choose a reason for hiding this comment

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

ah, good point

@segiddins
Copy link
Member

It didn't resolve the real world problem.

Which real-world problem? Does this only manifest on gem update, or also gem install? Can you add a test case for the changed behavior?

@bundlerbot
Copy link
Collaborator

☔ The latest upstream changes (presumably #2177) made this pull request unmergeable. Please resolve the merge conflicts.

@colby-swandale
Copy link
Member

ping @hsbt

@hsbt
Copy link
Member Author

hsbt commented Mar 1, 2018

Which real-world problem?

If you want to use the latest version of csv with Ruby 2.4.0 via gem install csv, but RubyGems will activate csv on standard libraries. It needs to be promoted default gems on Ruby 2.4.

def verify_default_gems
return unless options[:install_as_default]
return if Gem::Specification::DEFAULT_GEMS_LIST.include?(spec.name)
raise Gem::InstallError, "#{spec} is not a default gems"
Copy link
Member

Choose a reason for hiding this comment

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

not a default gem ?

hsbt added a commit that referenced this pull request Mar 3, 2018
strscan
webrick
zlib
] #:nodoc:
Copy link
Member

Choose a reason for hiding this comment

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

do we want to freeze this list, or otherwise append to it when we see more gems in the default directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I will reconsider to handle its list.

@deivid-rodriguez
Copy link
Member

deivid-rodriguez commented Sep 19, 2018

@hsbt I'm running into the following issue with bundler

$ gem list json

*** LOCAL GEMS ***

json (default: 2.1.0)
jsonapi-renderer (0.2.0)
multi_json (1.13.1)

$ echo -e "source 'https://rubygems.org'\ngem 'json'" > Gemfile

$ bundle
Bundle complete! 1 Gemfile dependency, 2 gems now installed.
Bundled gems are installed into `./.bundle`

$ bundle pristine
Failed to pristine json (2.1.0). Cached gem /home/deivid/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/cache/json-2.1.0.gem does not exist.

$ gem install json
Fetching: json-2.1.0.gem (100%)
Building native extensions. This could take a while...
Successfully installed json-2.1.0
1 gem installed

$ bundle pristine
Installing json 2.1.0 with native extensions

$ gem uninstall json
Successfully uninstalled json-2.1.0

$ gem list json

*** LOCAL GEMS ***

json (default: 2.1.0)
jsonapi-renderer (0.2.0)
multi_json (1.13.1)

$ bundle pristine
Failed to pristine json (2.1.0). Cached gem /home/deivid/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/cache/json-2.1.0.gem does not exist.

Do you think the root problem might be what this PR is trying to address?

@hsbt
Copy link
Member Author

hsbt commented Sep 26, 2018

@deivid-rodriguez Yes, The current default-gems was inconsistency behavior.

@deivid-rodriguez
Copy link
Member

Great! For starters I'm going to try and confirm whether this PR fixes the problem!

@deivid-rodriguez
Copy link
Member

I tried it with this PR and I still get the same behavior. Can you confirm the issue? Maybe I didn't try it correctly.

@hsbt
Copy link
Member Author

hsbt commented Oct 4, 2018

@deivid-rodriguez Sorry, I didn't understand that your expected behavior. Did you hope to finish gem pristine json?

@deivid-rodriguez
Copy link
Member

Yeah, I hope bundle pristine would not crash.

But I just tried gem pristine json and it behaves better

$ gem pristine json
Restoring gems to pristine condition...
Skipped json-2.1.0, it is a default gem

So, this seems like a bug in bundler, not in rubygems! Sorry for the false alarm.

@hsbt
Copy link
Member Author

hsbt commented Oct 4, 2018

@deivid-rodriguez Ah, I see. I missed bundle pristine on your first comment. It seems the issue of bundler.

@hsbt hsbt added this to the 4.0 milestone Oct 10, 2018
@deivid-rodriguez
Copy link
Member

@hsbt I'd like to move forward the work in this PR if that's ok with you?

ghost pushed a commit to rubygems/bundler that referenced this pull request Jun 24, 2019
7216: Revert "Migrate requires from exe/ to also be relative" r=hsbt a=deivid-rodriguez


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

The problem was that in #7193, I included [a commit](d9d2bf6) to migrate requires included in bundler's executable to use `require_relative`. That broke stuff.

### What was your diagnosis of the problem?

My diagnosis was the assumption that if `<install_folder>/exe/bundle` lives on a folder, the corresponding bundler lib lives on `<install_folder>/lib` doesn't hold for default gems. Default gems for gems with executables live in `site_lib` but install their executables in the standard gem location. That means that the reference commit breaks bundler when it is installed as a default gem.

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

My fix is to revert the commit.

### Why did you choose this fix out of the possible options?

I chose this fix because it's the easiest way. The proper long term fix is probably to make default gems behave in a more standard way. There's some ongoing work on that here: rubygems/rubygems#2166.


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
ghost pushed a commit that referenced this pull request Mar 11, 2020
7216: Revert "Migrate requires from exe/ to also be relative" r=hsbt a=deivid-rodriguez


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

The problem was that in rubygems/bundler#7193, I included [a commit](rubygems/bundler@d9d2bf6) to migrate requires included in bundler's executable to use `require_relative`. That broke stuff.

### What was your diagnosis of the problem?

My diagnosis was the assumption that if `<install_folder>/exe/bundle` lives on a folder, the corresponding bundler lib lives on `<install_folder>/lib` doesn't hold for default gems. Default gems for gems with executables live in `site_lib` but install their executables in the standard gem location. That means that the reference commit breaks bundler when it is installed as a default gem.

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

My fix is to revert the commit.

### Why did you choose this fix out of the possible options?

I chose this fix because it's the easiest way. The proper long term fix is probably to make default gems behave in a more standard way. There's some ongoing work on that here: #2166.


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
@rubygems rubygems deleted a comment Dec 27, 2022
@rubygems rubygems deleted a comment Dec 27, 2022
hsbt added a commit that referenced this pull request Mar 10, 2023
hsbt added a commit that referenced this pull request Mar 10, 2023
@hsbt
Copy link
Member Author

hsbt commented Mar 10, 2023

I reboot this proposal for altanative approach of https://bugs.ruby-lang.org/issues/19351

@hsbt hsbt force-pushed the install-default-gems branch 3 times, most recently from f566bd8 to a8347d7 Compare March 10, 2023 10:29
mahjelan

This comment was marked as spam.

@hsbt
Copy link
Member Author

hsbt commented Apr 18, 2024

I changed my proposal. see #7588

@hsbt hsbt closed this Apr 18, 2024
@hsbt hsbt deleted the install-default-gems branch April 18, 2024 07:46
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

8 participants