Fix check to disable LTO by default #192
Conversation
bradparker
changed the title
|
#148 is the original issue for this |
| @@ -27,7 +27,7 @@ | |||
| # darwin nix clang doesn't support lto | |||
| # disable -lto flag for darwin + nix | |||
| # see: https://github.com/sass/sassc-ruby/issues/148 | |||
| enable_lto_by_default = (Gem::Platform.local.os == "darwin" && !ENV['NIX_CC'].nil?) | |||
| enable_lto_by_default = (Gem::Platform.local.os == "darwin" && ENV['NIX_CC'].nil?) | |||
There was a problem hiding this comment.
On closer inspection this check is a little weird. Firstly I'm pretty sure nil? is a Rails addition. Secondly this should enable LTO by default unless we are using nix clang on darwin. Maybe:
enable_lto_by_default = !(Gem::Platform.local.os == "darwin" && ENV['NIX_CC'])
There was a problem hiding this comment.
Thanks for having a look at this :).
I think .nil? "comes standard".
~ $ irb
WARNING: This version of ruby is included in macOS for compatibility with legacy software.
In future versions of macOS the ruby runtime will not be available by
default, and may require you to install an additional package.
irb(main):001:0> nil.nil?
=> true
irb(main):002:0> "".nil?
=> false
irb(main):003:0>
I agree the check is weird and I think your version makes more sense. It reads well: "Enable LTO by default when we're not on Darwin and the nix wrapped C compiler will be used".
There was a problem hiding this comment.
Oh sorry about that, you're absolutely right!
Also just to make it more explicit, it looks like in master the only time LTO is automatically enabled is on Darwin with Nix clang, which is the opposite to what we want.
There was a problem hiding this comment.
Yep, sorry. The title of my branch / PR is weird :P. In my head it was "Fix check to disable the enabling of LTO by default" ... ergh.
I wonder if there isn't a better way to do this in general. Like to detect if LTO will work without having to know that it won't work for the C compiler wrapped by Nix on Darwin. Like, could it also not work with other C compilers? Is there some way to ask: "is LTO available?".
Until then it'd be good to get something like this merged as I'm sure other Darwin + Nix using Ruby devs are having a head-scratching time.
Update to ruby-sassc to 2.3.0. - **2.3.0** - [Fix rake gem:native task](sass/sassc-ruby#196) - [disable lto flag for darwin + nix](sass/sassc-ruby#166) - [Sort input file list](sass/sassc-ruby#178) - [Set appropriate encoding for source_map](sass/sassc-ruby#152) - [allow passing functions directly](sass/sassc-ruby#162) - [always dispose data context](sass/sassc-ruby#161) - [Update libsass to 3.6.3](sass/sassc-ruby#164) - [Restore strip symbols](sass/sassc-ruby#184) - [Default --march-tune-native to false](sass/sassc-ruby#158) - [Fix compile issue on Mac OS X](sass/sassc-ruby#174) - [Test on TruffleRuby in TravisCI](sass/sassc-ruby#171) - [Use RbConfig::MAKEFILE_CONFIG['DLEXT'] instead of hardcoding extensions](sass/sassc-ruby#173)
|
@glebm could you please review this if you get a moment? |
|
@bolandrm Care to release this as v2.4.1? |
|
I double checked. Could you please release the changes in this PR? Thank you! |
To include sass#192 into the release
|
That last comment was a year ago and the release did not happen 😢 FTR a workaround: |
|
@bolandrm I ran into this issue as well. Is there any way you can release a new version with the fix? |
This got sassc-ruby working for me using Nix on Darwin.