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.

Sign up for GitHub

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

Fix check to disable LTO by default #192

Merged
merged 1 commit into from
Jun 2, 2020
Merged

Fix check to disable LTO by default #192

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ext/extconf.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if enable_config('lto', enable_lto_by_default)
$CFLAGS << ' -flto'
Expand Down