PR#33: Add check for ruby package to prevent wrong behavior of "make install". - rpms/ruby

rpms / ruby

#33 Add check for ruby package to prevent wrong behavior of "make install".

Closed 5 years ago by vondruch. Opened 5 years ago by jaruga.

Unknown source feature/check-installed-ruby into master

Add check for ruby package to prevent wrong behavior of "make install".

Jun Aruga • 5 years ago

8df77da

ruby.spec

file modified

		`@@ -526,6 +526,13 @@`


		`%prep`
		`+ # Ensure ruby package is not install to run "make install" correctly.`
		`+ # Gem.default_dir in tool/rbinstall.rb in the process of "make install"`
		`+ # overridden by /usr/share/rubygems/rubygems/defaults/operating_system.rb`
		`+ # installs default gems to wrong path.`
		`+ # https://github.com/ruby/ruby/blob/v2_5_3/tool/rbinstall.rb#L734-L736`
		`+ rpm -q --quiet ruby && false`
		`+`
		`%setup -q -n %{ruby_archive}`

		`# Remove bundled libraries to be sure they are not used.`

jaruga commented 5 years ago

I want to add check if ruby package is not installed to ruby.spec file.

Because when normal user run src.rpm file created by the ruby.spec by rpmbuild command with installed ruby package, the build becomes error.

$ id
uid=28707(jaruga) gid=28707(jaruga) groups=28707(jaruga) ...

$ rpmbuild \
  --rebuild \
  --target=x86_64 \
  --rcfile /usr/lib/rpm/rpmrc:/usr/lib/rpm/redhat/rpmrc \
  ./*.src.rpm
...
+ make install DESTDIR=/home/jaruga/rpmbuild/BUILDROOT/ruby-2.5.3-102.fc28.x86_64
...
./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems -r./x86_64-linux-fake ./tool/rbinstall.rb --make="make" --dest-dir="/home/jaruga/rpmbuild/BUILDROOT/ruby-2.5.3-102.fc28.x86_64" --extout=".ext" --mflags="" --make-flags=" -- DESTDIR=/home/jaruga/rpmbuild/BUILDROOT/ruby-2.5.3-102.fc28.x86_64" --data-mode=0644 --prog-mode=0755 --installed-list .installed.list --mantype="doc" --install=all --rdoc-output=".ext/rdoc"
...
installing default gems from lib:   /usr/share/gems (build_info, cache, doc, extensions, gems, specifications)
...
+ mv /home/jaruga/rpmbuild/BUILDROOT/ruby-2.5.3-102.fc28.x86_64/usr/share/ruby/gems /home/jaruga/rpmbuild/BUILDROOT/ruby-2.5.3-102.fc28.x86_64/usr/share/gems
mv: cannot stat '/home/jaruga/rpmbuild/BUILDROOT/ruby-2.5.3-102.fc28.x86_64/usr/share/ruby/gems': No such file or directory
error: Bad exit status from /var/tmp/rpm-tmp.5mXeoU (%install)

Below line is wrong.

installing default gems from lib: /usr/share/gems (build_info, cache, doc, extensions, gems, specifications)

It should be like this.
installing default gems from lib: /usr/share/ruby/gems/ (build_info, cache, doc, extensions, gems, specifications)

This is Gem.default_dir overridden by installed /usr/share/rubygems/rubygems/defaults/operating_system.rb is used in tool/rbinstall.rb.
https://github.com/ruby/ruby/blob/v2_5_3/tool/rbinstall.rb#L734-L736

This check is good for someone to create binary RPMs by rpmbuild command.

After the modification

The check works.

$ rpmbuild \
  --rebuild \
  --target=x86_64 \
  --rcfile /usr/lib/rpm/rpmrc:/usr/lib/rpm/redhat/rpmrc \
  ./*.src.rpm
Installing ./ruby-2.5.3-102.fc30.src.rpm
Building target platforms: x86_64
Building for target x86_64
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.pgq6K8
+ umask 022
+ cd /home/jaruga/rpmbuild/BUILD
+ rpm -q --quiet ruby
+ false
error: Bad exit status from /var/tmp/rpm-tmp.pgq6K8 (%prep)

vondruch commented 5 years ago

Thank you for the PR. The intent is good. however, there are two reasons why I cannot accept this PR:

Calling rpm from rpm-build is a big no-no.
Installed Ruby is only one of many other ways to break Ruby build or to have some unexpected results. For example, the GDBM bindings can build against libdb, libdb4 and gdbm (not sure if the list is 100% correct). The build result will be different if C++ compiler is present. I CLang is installed on the system, it might be used instead of GCC. And so on and so on. There are a million ways to break the build. I don't see any reason to protect against on specific case. Actually, even if we had not operating_system.rb in place, the presence of Ruby might provide a different output, because installed Ruby is then going to be used on various places instead of miniruby (hint, this is the first check in configure script: checking for ruby... false)

Edited 5 years ago by vondruch

Pull-Request has been closed by vondruch

5 years ago

jaruga commented 5 years ago

"1." => sure. That makes sense. Below is the alternative. "command -v" is kind of which command and better way [1]

command -v ruby && false

"2."

Installed Ruby is only one of many other ways to break Ruby build or to have some unexpected results.

Maybe you know why I sent this PR, and the context.
I want to make someone who want to run ruby.spec by rpmbulid command, detect the reason of the error easily without people working for ruby.spec you, me or Pavel and etc.
Adding the check is to save our time.
I do not want to spend a time for the same issue's investigation in the future again with another person.

Why not check "many other ways" as much as possible in ruby.spec?

the GDBM bindings can build against libdb, libdb4 and gdbm (not sure if the list is 100% correct). The build result will be different if C++ compiler is present.

I do not know the GDBM buildings.
But we can add below logic to check "if C++ compiler is present" in ruby.spec?

command -v g++ && false

If CLang is installed on the system, it might be used instead of GCC.

We can Just set environment variable CC=gcc to specify the GCC in ruby.spec.

(hint, this is the first check in configure script: checking for ruby... false)

Sure. we might be able to check the output to installed Ruby.

How about just adding the check to ruby.spec, when people find "one of many other ways to break Ruby build or to have some unexpected results"?

$ git diff
diff --git a/ruby.spec b/ruby.spec
index 3e0845e..e7b3ace 100644
--- a/ruby.spec
+++ b/ruby.spec
@@ -526,6 +526,10 @@ HTTP.


 %prep
+CC=gcc
+command -v ruby && false
+command -v g++ && false
+
 %setup -q -n %{ruby_archive}

 # Remove bundled libraries to be sure they are not used.

[1] ShellCheck which is non-standard. Use builtin 'command -v' instead.: https://github.com/koalaman/shellcheck/wiki/SC2230

vondruch commented 5 years ago

There is no single reason I can think of for building the RPM outside of mock. As long as it builds in mock, then everything is OK. Thanks but no thanks.

pvalena commented 5 years ago

I agree with vondruch.

There is no need to restrain and overcomplicate ruby spec file on basis of our intended environment(simplest spec file is the best).

Ideally, IMO, Ruby should build in any environment with this spec file; or, alternatively, the autoconf should fail with error message about the environment being invalid.

Regarding our / Fedora case - maybe there's some documentation missing? (And if there is one, could it be linked in spec file?)