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

if MAKE or make are in the environment, use those instead of rbconfig's make. #443

Merged
merged 1 commit into from
Apr 4, 2013
Merged

if MAKE or make are in the environment, use those instead of rbconfig's make. #443

merged 1 commit into from
Apr 4, 2013

Conversation

erikh
Copy link
Contributor

@erikh erikh commented Feb 7, 2013

This is a solution for #426. I don't think there's an easy way to test it but am willing to write them if anyone has a good idea of how to.

It's very easy to get into this situation for non-linux, non-osx users, so I'm hoping this can make it in ASAP. The current situation is "rebuild ruby" for these situations, the converse (where someone has make in their environment and that breaks things because of this change) is much easier to resolve for the end-user, which is why I think this change is pretty safe.

make. necessary for those who built ruby with something other than
GNU make (gmake is required to build mkmf output)
@raggi
Copy link
Contributor

raggi commented Feb 7, 2013

Another workaround for you besides rebuild ruby is to monkey patch rbconfig via $RUBYOPT.

@evanphx
Copy link
Member

evanphx commented Feb 7, 2013

This feels wrong. Why isn't ruby built with the right make to begin with and thus have it in rbconfig.rb? Using a different make could result in different compilers being run, generate extensions that aren't loadable or crash (think default rules varying per make).

@erikh
Copy link
Contributor Author

erikh commented Feb 7, 2013

mkmf doesn't generate a Makefile compatible with solaris make (or likely anything but gmake). autoconf, however, has no such problem. therefore it's very easy to get into a situation where one builds ruby with a standard OS toolchain that's not compatible with the makefiles typically generated for building extensions. I'd be surprised if anything that doesn't host a gnu toolchain by default (e.g, *BSD) can't be trivially put into this situation.

This allows me to point it at gmake instead for building the extensions, no matter how it was built. @raggi's solution is basically the same thing but severely hacking it to avoid a patch that really puts nobody in any danger by default -- they still have to explicitly specify they want to change the make program they want to run.

I don't think your concerns about the build environment are an issue because mkmf (again, the common use case) puts a lot of energy into configuring the build environment from rbconfig, so while in theory you may be correct, in practice it's probably not an issue. Additionally, being able to specify it allows me to shoot myself in the foot with cognition instead of asking me to rebuild ruby, where in my specific case I'm depending on packages that have built with this version of make to remain portable (the GNU toolchain is not the standard toolchain on solaris).

Alternatively, we could backport make compatibility fixes to mkmf to who knows how far back and release a ton of ruby versions. I don't think that's as reasonable as letting me override make with the environment and using a new version of rubygems, especially because MRI's build system itself doesn't have this issue. Of course, if I do have an issue I'll have to do what you suggest anyway, but at least I have a reasonable option to try first.

@drbrain
Copy link
Member

drbrain commented Feb 8, 2013

I don't see any particular harm in this patch, but I think it should be delayed until after rubygems 2.0, it is too close to ruby 2.0 to include now.

@erikh
Copy link
Contributor Author

erikh commented Feb 8, 2013

works for me, i just had bandwidth and figured it was safe enough to not be fiddling with for the next two weeks. be well!

@erikh
Copy link
Contributor Author

erikh commented Mar 14, 2013

hey eric, just wondering, is this acceptable to merge now that the smoke has cleared?

@drbrain
Copy link
Member

drbrain commented Mar 14, 2013

I'd like to wait until the smoke is well-dispersed before adding new features to master. I have this pull request schedule for the next new features release though, perhaps another week or two.

@erikh
Copy link
Contributor Author

erikh commented Mar 15, 2013

alrighty. thanks for getting back to me.

On Thu, Mar 14, 2013 at 8:44 AM, Eric Hodel notifications@github.comwrote:

I'd like to wait until the smoke is well-dispersed before adding new
features to master. I have this pull request schedule for the next new
features release though, perhaps another week or two.


Reply to this email directly or view it on GitHubhttps://github.com//pull/443#issuecomment-14909897
.

@haus
Copy link

haus commented Apr 2, 2013

This bug hit me as well on Solaris. This would be a win.

drbrain added a commit that referenced this pull request Apr 4, 2013
Use MAKE or make from ENV over rbconfig's make.
@drbrain drbrain merged commit f2bad74 into rubygems:master Apr 4, 2013
drbrain added a commit that referenced this pull request Apr 4, 2013
drbrain added a commit that referenced this pull request Apr 4, 2013
drbrain added a commit that referenced this pull request Apr 4, 2013
* pietro-key_passphrase: (154 commits)
  Update missed tests for rebase of #447
  Fixed pull request number type for #461
  Improve documentation of DependencyInstaller
  Alphabetize Gem::DependencyInstaller
  Removed commented out DependencyInstaller code
  Alter #489 to use GEM_SPEC_CACHE
  fix tests when GEM_SPEC is set in environment
  add support for ENV GEM_SPEC, fix #430
  Updated history for #443
  Don't alter Gem::Specification.dirs during install
  Default to Gem.dir as late as possible.
  Updated history for #455
  Update history for #456
  Update history for #462
  Add tests and alter output of #514
  add PATH to gem env
  Update History for #514
  Restore backwards-compatibility for #517
  Alphabetize RequestSet
  Undent RequestSet
  ...

Conflicts:
	test/rubygems/test_gem_commands_cert_command.rb
	test/rubygems/test_gem_package.rb
drbrain added a commit that referenced this pull request Jun 27, 2013
drbrain added a commit that referenced this pull request Jun 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants