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

Fix broken brew formula due to loading operating_system.rb customizations too late #5154

Merged

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Dec 8, 2021

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

Hombrew ruby formula no longer works since #5044 .

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

The problem is that setting up default gems needs Gem.default_dir, which is something commonly customized in this file. So if we setup default gems before loading operating_system.rb customizations, we will setup the wrong default gems.

Unfortunately fixing this requires reverting #5044, but I wasn't even sure that enhancement was the right thing to do, and the culprit ended up being something else, so we should fix the real culprit there instead.

Fixes #5151.
Fixes #5043.

Make sure the following tasks are checked

@deivid-rodriguez deivid-rodriguez force-pushed the load_os_customization_before_default_dir_is_first_used branch 2 times, most recently from 3d9f18a to 60a2ac1 Compare December 9, 2021 05:21
lib/rubygems.rb Show resolved Hide resolved
@deivid-rodriguez deivid-rodriguez force-pushed the load_os_customization_before_default_dir_is_first_used branch from 9f760bd to c93bb1e Compare December 9, 2021 19:03
@deivid-rodriguez deivid-rodriguez mentioned this pull request Dec 10, 2021
@voxik
Copy link
Contributor

voxik commented Dec 10, 2021

  1) Failure:
GemTest#test_operating_system_customizing_default_dir [/builddir/build/BUILD/ruby-3.0.3/test/rubygems/test_rubygems.rb:39]:
Expected "/builddir/build/BUILD/ruby-3.0.3/ruby: No such file or directory -- gem (LoadError)" to be empty.

@voxik
Copy link
Contributor

voxik commented Dec 10, 2021

Just FTR, the test failure happened when I applied the patch to Ruby 3.0.3 and executed the test suite via make check. I am not sure that the gem command could be found on PATH in this configuration.

@voxik
Copy link
Contributor

voxik commented Dec 10, 2021

And again, I am not happy about the symlink expansion. This is not right. This is not intuitive.

Just for example, if you execute Ruby via /bin/ruby, then it is not acceptable Gems to be reported from /urs/lib. This causes just confusion.

If the symlink expansion would be so good idea, then this would be the shell behavior:

$ $ ll /
celkem 20
dr-xr-xr-x.   1 root root    0 22. zář 11.54 afs
lrwxrwxrwx.   1 root root    7 22. zář 11.54 bin -> usr/bin
dr-xr-xr-x.   7 root root 4096  4. lis 18.40 boot
drwxr-xr-x.  20 root root 4660 10. pro 09.31 dev
drwxr-xr-x.   1 root root 4716  8. pro 16.22 etc
drwxr-xr-x.   1 root root   16 22. zář 11.54 home
lrwxrwxrwx.   1 root root    7 22. zář 11.54 lib -> usr/lib
lrwxrwxrwx.   1 root root    9 22. zář 11.54 lib64 -> usr/lib64
drwx------.   1 root root    0 29. říj  2020 lost+found
drwxr-xr-x.   1 root root    0 22. zář 11.54 media
drwxr-xr-x.   1 root root   30 22. zář 11.54 mnt
drwxr-xr-x.   1 root root    0 22. zář 11.54 opt
dr-xr-xr-x. 389 root root    0 12. lis 10.25 proc
dr-xr-x---.   1 root root  206  1. pro 15.59 root
drwxr-xr-x.  51 root root 1340 10. pro 06.28 run
lrwxrwxrwx.   1 root root    8 22. zář 11.54 sbin -> usr/sbin
drwxr-xr-x.   1 root root    0 22. zář 11.54 srv
dr-xr-xr-x.  13 root root    0 12. lis 10.25 sys
drwxrwxrwt.  30 root root  780 10. pro 13.28 tmp
drwxr-xr-x.   1 root root  100  1. říj 10.29 usr
drwxr-xr-x.   1 root root  200  1. říj 10.36 var

$ cd /bin

$ pwd
/usr/bin

But in reality, the result is /bin.

Just as an example this is place, where previously used ln -s needed to be replaced by cp -a when require_relative was introduced into RDoc. This is the stuff we need to do in Fedora to be able to execute the test suite. Having multiple copies of test suite around is not better then symlink.

@voxik
Copy link
Contributor

voxik commented Dec 10, 2021

Forgot to mention that I have reported this ticket yesterday to solve the issue:

https://bugs.ruby-lang.org/issues/18401

The idea is to really make require_relative to behave the same as require without expanding the symlinks

@deivid-rodriguez deivid-rodriguez force-pushed the load_os_customization_before_default_dir_is_first_used branch from c93bb1e to c6a9c81 Compare December 10, 2021 13:19
@deivid-rodriguez
Copy link
Member Author

We have been expanding $LOAD_PATH entries in other places for a long time when we need to compare them with other paths, in order to fix issues with symlinked rubies. For example, here:

def clean_load_path
loaded_gem_paths = Bundler.rubygems.loaded_gem_paths
$LOAD_PATH.reject! do |p|
resolved_path = resolve_path(p)
next if $LOADED_FEATURES.any? {|lf| lf.start_with?(resolved_path) }
loaded_gem_paths.delete(p)
end
$LOAD_PATH.uniq!
end
def resolve_path(path)
expanded = File.expand_path(path)
return expanded unless File.respond_to?(:realpath) && File.exist?(expanded)
File.realpath(expanded)
end
. I think it was added because otherwise bundler was crashing when run on some rvm gemsets that use symlinks.

In general, I think avoiding runtime crashes or redefinition warnings is a higher priority than potentially causing confusion when symlinked rubies are used.

I updated the test so that it hopefully works in your environment now.

@voxik
Copy link
Contributor

voxik commented Dec 10, 2021

I updated the test so that it hopefully works in your environment now.

Yes, the test passes and it seems to fix also the issues #5156 👍 Going to apply this fix to Ruby in Fedora in a bit.

This class does not use `rubygems/deprecate`. It uses
`rubygems/version`, which in turn uses `rubygems/deprecate`. Make this
explicit.
These files are loaded on startup unconditionally, so we can require
them relatively when needed.
It's very common for packagers to configure gem paths in this file, for
example, `Gem.default_dir`. Also, setting up default gems requires these
paths to be set, so that we know which default gems need to be setup.

If we setup default gems before loading `operatin_system.rb`
customizations, the wrong default gems will be setup.

Unfortunately, default gems loaded by `operating_system.rb` can't be
upgraded if we do this, but it seems much of a smaller issue. I wasn't
even fully sure it was the right thing to do when I added that, and it
was not the culprit of the end user issue that led to making that
change.

So as part of this change we also need to:

* Revert the changes that led to adding the above functionality.
* Update some the version of fiddle installed by specs that need to
  customize `Gem.default_dir`.

For completeness, the explanation of why we need to do the latter, it's
because these specs were installing a version of fiddle to this
customized default dir that no longer matches the default fiddle version
on ruby 3.0.3. That means that while before this change, fiddle would
required after the default dir had been changed (and whichever version
installed there would be correctly picked up), now fiddle is required
and activated as a gem _before_ this folder is customized, and then will
fail to be found when eagerly resolving dependencies from bundler's
binstub. So we now need to install a version exactly matching the
highest default version in our support matrix to avoid that. This is
still brittle, but I won't be further addressing that here.
Some double load issues were reported a while ago by OS packagers where
if a gem has been required before rubygems, and then after, rubygems
require would cause a double load.

We avoid this issue by activating the corresponding gem if we detect
that a file in the default LOAD_PATH that belongs to a default gem has
already been required when rubygems registers default gems.

However, the fix does not take into account that the default LOAD_PATH
could potentially include symlinks. This change fixes the same double
load issue described above but for situations where the default
LOAD_PATH includes symlinks.
@deivid-rodriguez deivid-rodriguez force-pushed the load_os_customization_before_default_dir_is_first_used branch from c6a9c81 to 5029f9d Compare December 11, 2021 18:13
@deivid-rodriguez deivid-rodriguez merged commit 793ad95 into master Dec 12, 2021
@deivid-rodriguez deivid-rodriguez deleted the load_os_customization_before_default_dir_is_first_used branch December 12, 2021 01:00
hsbt pushed a commit that referenced this pull request Feb 3, 2023
…efault_dir_is_first_used

Fix broken brew formula due to loading `operating_system.rb` customizations too late
hsbt pushed a commit that referenced this pull request Feb 3, 2023
…efault_dir_is_first_used

Fix broken brew formula due to loading `operating_system.rb` customizations too late
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants