Created attachment 312651 [details, diff] Patch proposal to fix the stripping issue I encountered this when hacking sys-freebsd/freebsd-lib as part of a fix for bug #409269. The ebuild has multiple instances of append-flags and my changes added more. The current flag-o-matic.eclass code will strip existing flags that use spacing. That means that things like "-I /path" will get stripped. In addition, those specifying the flags that -march=native sets in CFLAGS will also see --param stripped, which affects distcc users. Lastly, any paths containing spaces willa also be stripped. I wrote a patch to address this, but I would like vapier's feedback before anything is done with it.
your patch could break testing of flags that need flags added earlier adding "-I /path" isn't supported ... just use "-I/path"
I think we also decided that --param wasn't worth it. How does that affect distcc?
(In reply to comment #1) > your patch could break testing of flags that need flags added earlier > > adding "-I /path" isn't supported ... just use "-I/path" Could you elaborate on which flags require this? What prevents them from being appended together? (In reply to comment #2) > I think we also decided that --param wasn't worth it. How does that affect > distcc? The use of distcc should not result in different binaries, but the stripping means that binaries are not strictly identical to their -march=native counterparts when distcc is used.
> The use of distcc should not result in different binaries Yes. > but the stripping > means that binaries are not strictly identical to their -march=native > counterparts when distcc is used. We certainly don't make any kind of guarantee that you putting something other than -march=native in your CFLAGS gets you the same as -march=native. distcc is only the reason you're not using -march=native, not the cause of the issue, correct? There's also bug #390537.
The main reason I don't use -march=native is because `emerge --info <atom>` does not provide as much useful information when packages are built with it.
True enough. I've been thinking about somehow expanding -march=native when logging. It would definitely save us some time. But that's another bug.
*** Bug 419407 has been marked as a duplicate of this bug. ***
(In reply to comment #1) > your patch could break testing of flags that need flags added earlier These functions invoke test-flags-PROG, which will then tokenize the flags by splitting them by whitespace. Each flag is individually passed to test-flag-PROG, where it is tested without any other flags present. The issue you describe should already be present in the existing code, assuming that such flags exist.
(In reply to comment #2) > I think we also decided that --param wasn't worth it. How does that affect > distcc? Does gcc have one-parameter equivalent of 2-parameter things like --param l1-cache-line-size=64
> Does gcc have one-parameter equivalent of 2-parameter things like > > --param l1-cache-line-size=64 Looks like --param=l1-cache-line-size=64 does the same thing.
Comment on attachment 312651 [details, diff] Patch proposal to fix the stripping issue this only solves part of the problem. it doesn't fix the case like: append-cxxflags -isystem /some/path
this issue was started with: http://sources.gentoo.org/eclass/flag-o-matic.eclass?r1=1.156&r2=1.157 there's no bug referenced there, so i'm not sure if Ryan had one in mind ... in the past, the canonical form for a possibly-unsupported-flag was: append-flags $(test-flags-CC -Wa,--noexecstack) i wonder if we should just revert 1.157 and update the documentation for these funcs to suggest the $(test-flags-XXX) in these cases. Ryan: WDYT ?
Well there's about 6 packages in the tree that do it properly now so I don't have high hopes. :) It'd be nice if we could validate these flags somehow. IIRC the motivation for the change was after the dozenth time I had to add version guards around `append-flags -mno-avx` in people's ebuilds back when we were having all those x86 avx bugs with gcc-4.4. Your patch in comment #11 is correct though, it shouldn't have been testing every flag, just the one appended. My mistake. But yeah, if append-flags has to support adding multiple flags in one call then I don't see how we can tell the difference between that and flags with spaces. Maintaining a list of flags that can contain whitespace is not an option (there are 194 of them in 4.8.1 *). It's probably better to have it do what the dev expects it to do and just add the flag as-is. strip-unsupported-flags has the same problem. Anything with spaces will get wiped out. * for x in $(find . -iname *.opt); do grep -B1 Separate $x; done
(In reply to Ryan Hill from comment #13) i'm more of the opinion that maintainers doing things like adding -mno-avx to their ebuilds are simply doing it wrong. that kind of junk really has no business being in ebuilds in the first place. i'll change these funcs back to just adding things as-is. when someone hits a problem, it's then the maintainers problem to unscrew their ebuild. we probably should send a PSA/reminder on the topic to the dev list. i'm not too worried about strip-unsupported-flags as that is generally a sledge hammer in limited circumstances (like cross-compiling). a quick scan of the tree shows <20 packages using it, and they're almost all toolchain related.
(In reply to SpanKY from comment #14) ok, i've changed things back and updated the comments/documentation: http://sources.gentoo.org/eclass/flag-o-matic.eclass?r1=1.188&r2=1.189
