GN : Add cflag for all non-test targets w/ exceptions?

[Gabriel Charette]

Hello all,

I'd like to enable -Wglobal-constructors (crbug.com/801563).

But it's not suitable for tests (gtest and other reasons).

And I only care about using it to protect against inadvertently adding static initializers in the product.

I also need to be able to add exceptions as there are various third_party components that already don't comply.

Is there a way to add a product-only (i.e. all targets not marked test_only?) cflag in GN? As well as override it (perhaps with -Wno-global-constructors) in third_party components that violate it today.

Thanks!

Gab

[Nico Weber]

Put the flag in a config. Add it to the default config list. Remove it again in testing/test.gni (ctrl-f "configs" in that file). Remove it from needed 3p configs. Rough example: https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?type=cs&q=extra_warnings+file:%5C.gn&sq=package:chromium&l=572 and references (see query).

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LK4qkYhTUyjCMZd2bh9poxmAVzf27rxr9SwE8%3DKwFJXqQ%40mail.gmail.com.

[Nico Weber]

I just realized that this won't help you with test-only code that's e.g. in a static_library() instead of a test() :-/

[Dirk Pranke]

Yeah, there's no bulletproof way to do what you want.

We currently have test steps to check for static initializers; I realize that's not quite as nice as a compiler flag, but I'd guess it should be sufficient?

-- Dirk

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiFRb_u%2Bc2eFaqz1H3%2BBEwSneAU8fZkHQ1PLg5jkrAG1iw%40mail.gmail.com.

[Primiano Tucci]

On Fri, Jan 12, 2018 at 4:03 PM Nico Weber <tha...@chromium.org> wrote:

I just realized that this won't help you with test-only code that's e.g. in a static_library() instead of a test() :-/

Even in that case, wouldn't such a static_library be marked with testonly=true? What are the cases where we have a test-only translation unit in a non testonly target? I'd be surprised if we had gtest fixtures (I think we are secretly talking about those, which need static initializers) linked in production code.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiFRb_u%2Bc2eFaqz1H3%2BBEwSneAU8fZkHQ1PLg5jkrAG1iw%40mail.gmail.com.

[Dirk Pranke]

On Fri, Jan 12, 2018 at 10:25 AM, Primiano Tucci <prim...@chromium.org> wrote:

On Fri, Jan 12, 2018 at 4:03 PM Nico Weber <tha...@chromium.org> wrote:

I just realized that this won't help you with test-only code that's e.g. in a static_library() instead of a test() :-/

Even in that case, wouldn't such a static_library be marked with testonly=true? What are the cases where we have a test-only translation unit in a non testonly target? I'd be surprised if we had gtest fixtures (I think we are secretly talking about those, which need static initializers) linked in production code.

There aren't any; GN won't let you do that.

However, I think the point was is that if you have a test binary, you're using the test() template and we can remove the config there (in one place). We can't do that for a vanilla test-only static_library/source_set/etc. so, we'd have to replace them with test_static_library(), test_source_set(), etc.

-- Dirk 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2ByH71cvnQeDnquXpCJggytQ54kaaagmjVkLrZy1RqcSnBMguQ%40mail.gmail.com.

[Primiano Tucci]

On Fri, Jan 12, 2018 at 10:21 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Fri, Jan 12, 2018 at 10:25 AM, Primiano Tucci <prim...@chromium.org> wrote:

On Fri, Jan 12, 2018 at 4:03 PM Nico Weber <tha...@chromium.org> wrote:

I just realized that this won't help you with test-only code that's e.g. in a static_library() instead of a test() :-/

Even in that case, wouldn't such a static_library be marked with testonly=true? What are the cases where we have a test-only translation unit in a non testonly target? I'd be surprised if we had gtest fixtures (I think we are secretly talking about those, which need static initializers) linked in production code.

There aren't any; GN won't let you do that.

However, I think the point was is that if you have a test binary, you're using the test() template and we can remove the config there (in one place). We can't do that for a vanilla test-only static_library/source_set/etc. so, we'd have to replace them with test_static_library(), test_source_set(), etc.

Ahh got it. Didn't decode the past msg properly. Right, the "Remove it again in testing/test.gni " is the NotThatEasy™ part and requires changing the various test_support targets manually.

[Gabriel Charette]

I'm still not clear on why we can't remove from all test configs but can we add to all? i.e. will adding the overridding -Wno-global-constructors flag work?

[Sylvain Defresne]

The way to add to all target is via set_defaults. This gn directive takes a target type name and a scope and assign all the value defined in the scope to all targets of that type (this is why you need to do configs += []).

The issue is that both non test and test targets can both be of the same target type, for example:

set_default("source_set") {

  configs = [ "//build/compiler:default_config" ]

}

source_set("foo") {

  sources = [ ... ]

}

source_set("foo_tests") {

  testonly = true

  sources = [ ... ]

  deps = [ ":foo" ]

}

test("some_unittests") {

  testonly = true

  deps = [

    ":foo_tests",

    ":bar_tests",

  ]

}

In "some_unittests" target we cannot remove flags from dependent targets ("foo_tests", "bar_tests"). And set_defaults only allow setting the same default config to both "foo" and "foo_tests" targets.

If we add -Wno-global-constructors to default_config then both production ("foo") and test ("foo_tests") will get it. Same for -Wglobal-constructors.

The way to do this, is as suggested, to introduce test_source_set (and test_static_library, test_shared_library, ...) templates that add the correct config.

Hope this helped.

-- Sylvain

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LKVPf%2Bpqa5u0iQ9Ois%3DDoZ55G4muM0ZNXZ%2BuWrLfnbtTA%40mail.gmail.com.

[Nico Weber]

Before doing that, I'd try doing what I recommended, and in addition manually removing the config for test-only source sets as needed. Maybe there aren't so many of them. (And global ctors/dtors in tests aren't super great either, and since it's easy to make a source_set() that is currently test-only be not test-only later, not having too many global ctors/dtors in source sets is probably a good idea anyhow.)