Fix DESTDIR related 'make' issue when DESTDIR specified #327

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@repeatedly

make has an order which applies environment variable.
If we want to disable existence environment variable in Makefile,
then using make's argument is better.

I hit this issue when try to make td-agent RPM package with Ruby 2.0 (this report seems related: https://bugzilla.redhat.com/show_bug.cgi?id=921650 ).
I don't know the better solution but this bug is critical for our RPM packaging.

And maybe, 2.1 has same bug.

@repeatedly repeatedly Fix DESTDIR related 'make' issue when DESTDIR specified
make has an order which applies environment variable.
If we disable existence environment in Makefile,
then using make's argument is better.
dc833b8
@hsbt
Ruby Programming Language member

@drbrain Please review this pull request

@knu
Ruby Programming Language member

I'd like to see the Makefile that is causing the problem. Does it define DESTDIR (when it shouldn't) ?

@knu
Ruby Programming Language member

I mean, if Makefile defines DESTDIR inside, make DESTDIR= does not help anyway e.g. on *BSD make.

@repeatedly

I don't know the details of *BSD make so I don't know the best solution.
If there is more better approach, please use!

The important thing for us is to fix this critical bug :)

@knu
Ruby Programming Language member

I guess no one has responded to this issue because they don't see what the problem is and how this is a bug on the rubygems side.

To be specific, how an unexpected DESTDIR value comes in? That's why I asked for a copy of the Makefile in question.

@nagachika
Ruby Programming Language member

ping.
Can I merge it into CRuby repository?

@repeatedly

@knu Installation flow of td-agent is make install DESTDIR=$RPM_BUILD_ROOT -> gem install -> make install (specifying DESTDIR is popular RPM manner).
In RPM process, DESTDIR and other environment variables are assigned into MAKEFLAGS, so DESTDIR is not cleared at last make install.
So maybe, defining override DESTDIR = in generated Makefile or my patch resolves this issue.

This issue is introduced since Ruby 2.0, so rubygems / mkmf.rb breaks a compatibility.
If this is not a bug of ruby side, need packging guide to avoid DESTDIR problem of rubygems.

@knu
Ruby Programming Language member

Ah, MAKEFLAGS. Now I see the problem.

So, why not always pass DESTDIR as below?
I also changed the order of arguments as nmake may not allow macro definitions after a target name.

diff --git a/lib/rubygems/ext/builder.rb b/lib/rubygems/ext/builder.rb
index 9b6ad30..ab454b4 100644
--- a/lib/rubygems/ext/builder.rb
+++ b/lib/rubygems/ext/builder.rb
@@ -23,9 +23,14 @@ class Gem::Ext::Builder
       make_program = (/mswin/ =~ RUBY_PLATFORM) ? 'nmake' : 'make'
     end

-    ['', ' install'].each do |target|
-      cmd = "#{make_program}#{target}"
-      run(cmd, results, "make#{target}")
+    ['', 'install'].each do |target|
+      # Pass DESTDIR via command line to override what's in MAKEFLAGS
+      cmd = [
+        make_program,
+        '"DESTDIR=%s"' % ENV['DESTDIR'],
+        target
+      ].join(' ').rstrip
+      run(cmd, results, "make #{target}".rstrip)
     end
   end
@knu
Ruby Programming Language member

I'll fix the tests and send a pull request to the upstream (rubygems/rubygems) later.

@repeatedly

@knu I tested your patch and works fine. Thanks!

@nobu
Ruby Programming Language member

I think it is a bug in mkmf.rb that DESTDIR is embedded unexpectedly.

@knu
Ruby Programming Language member

@nobu I'm afraid there is nothing mkmf.rb can or should do here, because in principle DESTDIR is for "make install" and it may not necessarily be specified (possibly through MAKEFLAGS) in "ruby extconf.rb" or "make".

So, I think it's only the driver (rubygems in this context) that can and should take full care of any variable that might affect the installation process.

@knu knu added a commit that closed this pull request Jun 25, 2013
@knu knu * lib/rubygems/ext/builder.rb (Gem::Ext::Builder.make): Pass
  DESTDIR via command line to override what's in MAKEFLAGS.  This
  fixes an installation problem under a package building
  environment where DESTDIR is specified in the (parent) command
  line. [Fixes GH-327]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@41629 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
8cc3103
@knu knu closed this in 8cc3103 Jun 25, 2013
@evanphx evanphx pushed a commit that referenced this pull request Jun 25, 2013
@nagachika nagachika merge revision(s) 41629: [Backport #8533]
	* lib/rubygems/ext/builder.rb (Gem::Ext::Builder.make): Pass
	  DESTDIR via command line to override what's in MAKEFLAGS.  This
	  fixes an installation problem under a package building
	  environment where DESTDIR is specified in the (parent) command
	  line. [Fixes GH-327]


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_0_0@41635 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
fb82ba2
@amatsuda amatsuda pushed a commit to amatsuda/rubygems that referenced this pull request Nov 29, 2013
@knu knu Pass DESTDIR via command line to override what's in MAKEFLAGS.
This fixes an installation problem under a package building
environment where DESTDIR is specified in the (parent) command line.

Related bug reports:
- https://bugzilla.redhat.com/show_bug.cgi?id=921650
- ruby/ruby#327
a326585
@tenderlove tenderlove pushed a commit to tenderlove/ruby that referenced this pull request Jan 24, 2014
@knu knu * lib/rubygems/ext/builder.rb (Gem::Ext::Builder.make): Pass
  DESTDIR via command line to override what's in MAKEFLAGS.  This
  fixes an installation problem under a package building
  environment where DESTDIR is specified in the (parent) command
  line. [Fixes GH-327]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@41629 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
bd914d3
@tpawnell tpawnell pushed a commit to tpawnell/https-github.com-rubygems-rubygems that referenced this pull request Aug 13, 2015
@knu knu Pass DESTDIR via command line to override what's in MAKEFLAGS.
This fixes an installation problem under a package building
environment where DESTDIR is specified in the (parent) command line.

Related bug reports:
- https://bugzilla.redhat.com/show_bug.cgi?id=921650
- ruby/ruby#327
20d091a
@tpawnell tpawnell pushed a commit to tpawnell/https-github.com-rubygems-rubygems that referenced this pull request Aug 13, 2015
@knu knu Pass DESTDIR via command line to override what's in MAKEFLAGS.
This fixes an installation problem under a package building
environment where DESTDIR is specified in the (parent) command line.

Related bug reports:
- https://bugzilla.redhat.com/show_bug.cgi?id=921650
- ruby/ruby#327
75571d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment