merge flags when the second one does not have a `-`

[wolfv]

I ran into some problems with packages such as nokogiri and flags like CFLAGS=-isystem /foo.

The packages run something like append_cflags(ENV["CFLAGS"].split) unless ENV["CFLAGS"].nil?.

This fails because the /foo would be stripped as it is not a valid CFLAG alone.

That in turn leads to some compilation issues later.

I never coded in Ruby, so I am sure this solution isn't really good. Maybe some one can come up with a better patch for this issue. The idea is to look at the next argument, and figure out if it does not start with a - to merge it with the previous one.

[nobu]

Is only words that do not start with - arguments? Can't words starting with - be arguments?

Anyway, append_cflags(%w"-isystem /foo") raises a TypeError with this PR.

Rather I think it should be ENV["CFLAGS"].strip.split(/\s+(?=-|\z)/).

[wolfv]

Thanks @nobu for taking a look! Unfortunately, it's not something I can control. A few Ruby packages are doing this, e.g. Nokogiri: https://github.com/sparklemotion/nokogiri/blob/cee2c4dace0445e535aee0175afc0e10ecb193bc/ext/nokogiri/extconf.rb#L673

[casperisfine]

FYI @flavorjones, perhaps it's Nokogiri's extconf.rb that should be changed?

[wolfv]

FYI there are a couple of those in the wild: https://github.com/search?q=append_cflags%28ENV%5B%22CFLAGS%22%5D.split%29&type=code :)

[flavorjones]

Is there a reason that you can't pass in a CFLAGS string without spaces in your use case? that is:

CFLAGS=-isystem=/foo gem install nokogiri

The Nokogiri extconf can certainly change this line:

append_cflags(ENV["CFLAGS"].split) unless ENV["CFLAGS"].nil?

to this

append_cflags(ENV["CFLAGS"]) unless ENV["CFLAGS"].nil?

however, then trying the cflags from the environment variable is all-or-nothing, versus the current code which will try each in turn, and accept or reject each one based on compiler success or failure.

For example, if I run:

CFLAGS="-O1 -not-a-flag -Wmisleading-indentation" gem install nokogiri --platform ruby --verbose

Then I'll see this in the output:

...
checking for whether -O1 is accepted as CFLAGS... yes
checking for whether -not-a-flag is accepted as CFLAGS... no
checking for whether -Wmisleading-indentation is accepted as CFLAGS... yes
...

If someone can suggest a better way to do this, I'm happy to update the gems I maintain which includes nokogiri.

[wolfv]

I don't really understand the need to check each flag set by the env one-by-one. It seems like overriding the intention of the user (and breaks in this case and a number of others, such as -I foo, as pointed out in this comment: https://github.com/conda-forge/clang-compiler-activation-feedstock/pull/132#issuecomment-2203168631

My idea for a better way was to only split at -, so that you get flags + argument (if present).

[flavorjones]

@wolfv As the maintainer of Nokogiri who has to deal with users asking my why their CFLAGS aren't taking effect ... I guess we will have to agree to disagree on whether this behavior is needed or not.

The real long-term solution here is for mkmf to offer a better way to import compiler flags.

[flavorjones]

Is there a reason that you can't pass in a CFLAGS string without spaces in your use case?

I'm still curious about this.

checking for whether -isystem=/foo is accepted as CFLAGS... yes
checking for whether -I=foo is accepted as CFLAGS... yes
checking for whether -Ibar is accepted as CFLAGS... yes

Seems like a very easy workaround.

[flavorjones]

See https://github.com/sparklemotion/nokogiri/pull/3274 which will make it into Nokogiri v1.17.0

[isuruf]

-isystem=/foo and -isystem /foo are slightly different depending on whether the compiler uses a different sysroot or not. -isystem/foo is equivalent to -isystem /foo and is possible, but it's slightly unreadable and hence the preference to use -isystem /foo