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

--build-root option is not respected anymore #7083

Closed
voxik opened this issue Oct 20, 2023 · 14 comments · Fixed by #7212
Closed

--build-root option is not respected anymore #7083

voxik opened this issue Oct 20, 2023 · 14 comments · Fixed by #7212
Labels

Comments

@voxik
Copy link
Contributor

voxik commented Oct 20, 2023

This is command we regularly use on Fedora during build of PRM packages:

$ gem install -V --local --build-root . --force --document=ri,rdoc json-2.6.3.gem
Defaulting to user installation because default installation directory (/usr/share/gems) is not writable.
WARNING:  You build with buildroot.
  Build root: /builddir/build/BUILD/json-2.6.3
  Bin dir: /builddir/build/BUILD/json-2.6.3/builddir/.local/share/gem/ruby/bin
  Gem home: /builddir/build/BUILD/json-2.6.3/builddir/.local/share/gem/ruby
  Plugins dir: /builddir/build/BUILD/json-2.6.3/builddir/.local/share/gem/ruby/plugins
/builddir/build/BUILD/json-2.6.3/builddir/.local/share/gem/ruby/gems/json-2.6.3/CHANGES.md
/builddir/build/BUILD/json-2.6.3/builddir/.local/share/gem/ruby/gems/json-2.6.3/LICENSE
/builddir/build/BUILD/json-2.6.3/builddir/.local/share/gem/ruby/gems/json-2.6.3/README.md
/builddir/build/BUILD/json-2.6.3/builddir/.local/share/gem/ruby/gems/json-2.6.3/VERSION
/builddir/build/BUILD/json-2.6.3/builddir/.local/share/gem/ruby/gems/json-2.6.3/ext/json/ext/fbuffer/fbuffer.h

... snip ...

However, this should not install anything into ~/.local, but is should create the regular /usr/share/gems in the directory specified by the --build-root option.

@duckinator This is caused by #5327

Unless there is some straight forward fix, I'd suggest to revert the PR temporary (or I'll need to revert this feature for Fedora until this is fixed :/ )

@voxik voxik added the RubyGems label Oct 20, 2023
@voxik
Copy link
Contributor Author

voxik commented Oct 20, 2023

Hm, OTOH, maybe we should change stuff on Fedora side. Because this really changes defaults. But how?

@voxik
Copy link
Contributor Author

voxik commented Oct 20, 2023

This might be the biggest difference actually:

elsif File.writable?(Gem.default_dir)

The thing is that the writability of the directory was never checked at this place. And while the Gem.default_dir is not writable, the @build_root + Gem.default_dir would be writable (in theory, because the directory structure does not likely exists at this stage, not sure).

@duckinator
Copy link
Member

In general, I think if someone is altering the installation directory it should not be changed. It seems like RubyGems has a more fundamental problem that I hadn't noticed: there is not a single source of truth for the installation directory.

It looks like --build-root/@build_root acts as a prefix for @bin_dir, @gem_home, and @plugins_dir.

The problem is that the part of the code that does the auto-user-install check does not have access to the command-line arguments and runs before the code that handles --build-root.

The only solutions I can think of are:

  1. Have a truly global equivalent of --build-root. (Maybe an environment variable?)
  2. Adding some kind of "escape hatch" to the disable auto-user-install functionality without changing GEM_HOME.

@duckinator
Copy link
Member

One potential implementation of an "escape hatch" would be what I mentioned in #5327 (comment).

@voxik voxik changed the title --build-root oprion is not respected anymore --build-root option is not respected anymore Oct 22, 2023
@voxik
Copy link
Contributor Author

voxik commented Oct 22, 2023

Just FTR, this is my WIP attempt to make the --build-root prefixing the paths more visible:

voxik@3bf4652

My next step is to remove this check:

def check_install_dir # :nodoc:
if options[:install_dir] && options[:user_install]
alert_error "Use --install-dir or --user-install but not both"
terminate_interaction 1
end
end

And simply give the --install-dir higher priority then --user-install, because:

  1. In Fedora, we are forced to use --user-install everywhere and currently we need to specify --no-user-install if --install-dir is used, which is awful.
  2. There is no reason why --install-dir should not be the overriding option.

All this in an attempt to make the --user-install less exceptional.

@duckinator
Copy link
Member

Earlier attempts at #5327 encountered that exact --install-dir/--user-install conflict, and it was super frustrating. So I'm definitely in favor of ANYTHING that improves that situation.

Personally, I think having --user-install simply be a shorthand for --install-dir #{Gem.user_dir} makes sense. The way --user-install is implemented now feels very fragile, which is why the "auto-user-install" functionality doesn't use --user-install at all.

Also, a quick note: since you're explicitly setting --bindir anyway, --user-install and --install-dir #{Gem.user_dir} --bindir #{File.join [Dir.home, 'bin']} should be equivalent.

@darix
Copy link

darix commented Nov 14, 2023

This change caused a regression for us when building RPMs for openSUSE.

  1. the warning about "switching to user dir does not come"
ruby.ruby3.3 -r rubygems -e 'p Gem::Specification.new.cache_dir'
"/home/abuild/.local/share/gem/ruby/3.3.0+0/cache"
  1. there is no way to query the system directory anymore from the Gem::Specification instance.
  2. the assumption to force user directory when the Gem.default_dir is not writable is not interacting well with the --build-root option for gem install either:
install cmdline: ["/usr/bin/gem.ruby3.3", "install", "--verbose", "--local", "--build-root", "/home/abuild/rpmbuild/BUILDROOT/rubygem-activesupport-7.0-7.0.8-0.x86_64", "-f", "--no-document", "/home/abuild/rpmbuild/SOURCES/activesupport-7.0.8.gem"]

in this case the target directory is actually writable as it is prefixed with the build-root directory.

The whole code for when to use user dir in rubygems/installer.rb is partially obsoleted by the path_support change.

  def process_options # :nodoc:
    @options = {
      :bin_dir => nil,
      :env_shebang => false,
      :force => false,
      :only_install_dir => false,
      :post_install_message => true,
    }.merge options

    @env_shebang         = options[:env_shebang]
    @force               = options[:force]
    @install_dir         = options[:install_dir]
    @ignore_dependencies = options[:ignore_dependencies]
    @format_executable   = options[:format_executable]
    @wrappers            = options[:wrappers]
    @only_install_dir    = options[:only_install_dir]

    @bin_dir             = options[:bin_dir]
    @development         = options[:development]
    @build_root          = options[:build_root]

    @build_args = options[:build_args]

    @gem_home = @install_dir
    @gem_home ||= options[:user_install] ? Gem.user_dir : Gem.dir

    # If the user has asked for the gem to be installed in a directory that is
    # the system gem directory, then use the system bin directory, else create
    # (or use) a new bin dir under the gem_home.
    @bin_dir ||= Gem.bindir(@gem_home)
  
    @plugins_dir = Gem.plugindir(@gem_home)
 
    unless @build_root.nil?
      @bin_dir = File.join(@build_root, @bin_dir.gsub(/^[a-zA-Z]:/, "")) 
      @gem_home = File.join(@build_root, @gem_home.gsub(/^[a-zA-Z]:/, ""))   
      @plugins_dir = File.join(@build_root, @plugins_dir.gsub(/^[a-zA-Z]:/, ""))
      alert_warning "You build with buildroot.\n  Build root: #{@build_root}\n  Bin dir: #{@bin_dir}\n  Gem home: #{@gem_home}\n  Plugins dir: #{@plugins_dir}"
    end
  end

yes you can workaround it with export GEM_HOME="$(ruby.ruby3.3 -r rubygems -e 'puts Gem.default_dir')"

but this still feels like a regression in combination with the --build-root option.

@duckinator
Copy link
Member

@darix it's definitely a regression/bug. Sorry it's causing you problems.

@voxik does your work at #7100 resolve this, or is there still an outstanding problem after that PR?

I'm comparing v3.4.22 to #7100 and I'm noticing that https://github.com/rubygems/rubygems/pull/7100/files#diff-b25097522a961ed7158012499383b8a8057caf87cbc445d6b168a5b8a27ba45dL192-L197 and the changes to lib/rubygems/commands/install_command.rb just... weren't applied for v3.4.22 for some reason? But I'm unsure how vital those changes were.

@voxik
Copy link
Contributor Author

voxik commented Nov 30, 2023

This is the culprit:

elsif File.writable?(Gem.default_dir)

Having existing directory was previously not necessary.

@deivid-rodriguez
Copy link
Member

@voxik @darix Can you verify whether this is still happening in current master, and if it is, whether #7212 fixes it?

@duckinator
Copy link
Member

duckinator commented Nov 30, 2023

Confirmed it still happens as of 9766fa6 (current master).

puppy@orthrus:~/test$ gem install -V --local --build-root . --force --document=ri,rdoc json-2.6.3.gem
Defaulting to user installation because default installation directory (/usr/local/lib/ruby/gems/3.1) is not writable.
WARNING:  You build with buildroot.                                                                                                                                                                                Build root: /usr/home/puppy/test
  Bin dir: /usr/home/puppy/test/usr/home/puppy/.gem/ruby/3.1/bin                                                                                                                                                   Gem home: /usr/home/puppy/test/usr/home/puppy/.gem/ruby/3.1                                                                                                                                                      Plugins dir: /usr/home/puppy/test/usr/home/puppy/.gem/ruby/3.1/plugins
/usr/home/puppy/test/usr/home/puppy/.gem/ruby/3.1/gems/json-2.6.3/CHANGES.md
/usr/home/puppy/test/usr/home/puppy/.gem/ruby/3.1/gems/json-2.6.3/LICENSE
/usr/home/puppy/test/usr/home/puppy/.gem/ruby/3.1/gems/json-2.6.3/README.md
/usr/home/puppy/test/usr/home/puppy/.gem/ruby/3.1/gems/json-2.6.3/VERSION
/usr/home/puppy/test/usr/home/puppy/.gem/ruby/3.1/gems/json-2.6.3/ext/json/ext/fbuffer/fbuffer.h

<snip>

@duckinator
Copy link
Member

@deivid-rodriguez #7212 does not fix this. I do think it's a better overall approach than what I did, though, so I'll see if I can fix it on top of that PR instead of on top of master.

@deivid-rodriguez
Copy link
Member

Sorry, I misunderstood this issue, I can repro now 👍.

@duckinator
Copy link
Member

@voxik @darix I made a proposed change to @deivid-rodriguez's PR (#7212) that should fix this. The plan is to get some tests in place for that, then hopefully merge it tomorrow. 🙂

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

Successfully merging a pull request may close this issue.

4 participants