Re: [chromium-dev] Re: Use BUILDFLAG for OS checks?

[Xiaohan Wang (王消寒)]

This is already on chromium-dev for a week.

+embedd...@chromium.org: We are considering replacing defined(OS_XXX) with BUILDFLAG(IS_XXX), initially with the following:

```

BUILDFLAG(IS_ANDROID)
BUILDFLAG(IS_CHROMEOS)
BUILDFLAG(IS_FUCHSIA)
BUILDFLAG(IS_IOS)
BUILDFLAG(IS_LINUX)
BUILDFLAG(IS_MAC)
BUILDFLAG(IS_NACL)
BUILDFLAG(IS_WIN)
BUILDFLAG(IS_APPLE)
BUILDFLAG(IS_POSIX)

```

See the design doc has more detailed discussion. The prototype CL is here and the work is tracked in https://bugs.chromium.org/p/chromium/issues/detail?id=1234043. Thoughts and insights are welcome!

Best,

Xiaohan

On Thu, Aug 5, 2021 at 9:33 PM Avi Drissman <a...@google.com> wrote:

Do you want to run this by chromium-dev and embedder-dev? Run it by a larger audience, make sure no one can think of anything preventing it from happening, give a heads-up to everyone downstream.

On Thu, Aug 5, 2021 at 6:54 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

I think we reached consensus in the design doc so I'll move forward with the plan.

On Mon, Aug 2, 2021 at 12:50 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

As a reminder, we are considering replacing defined(OS_WIN) with BUILDFLAG(IS_WIN), the design doc has more detailed discussion. The prototype CL is here and the work is tracked in https://bugs.chromium.org/p/chromium/issues/detail?id=1234043. Thoughts and insights are welcome!

If there's no objections, I'll clean up the prototype CL and start working on this (by following the implementation plan in the doc).

On Thu, Jul 29, 2021 at 10:55 PM Bruce Dawson <bruce...@google.com> wrote:

The set of possible errors with the current setup is, apparently, endless. For instance, until recently PRESUBMIT.py would check that you had included build_config.h but it didn't actually check that you included it before using the defines. That led to errors like this one.

It is a lot of churn to change how this works, but it seems like a reasonable idea.

On Thursday, July 29, 2021 at 9:48:07 AM UTC-7 pb...@chromium.org wrote:

FWIW, from the sidelines I like this. This also saves us from typos like defined(OS_WINDWS).

On Thu, Jul 29, 2021 at 7:58 AM <dan...@chromium.org> wrote:

On Wed, Jul 28, 2021 at 8:30 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

I played with it a bit more and my recommendation is BUILDFLAG(IS_WIN). I wrote a short design doc on why I came up with this, and the plan I have in mind. The prototype CL is here. Please take a look and let me know what you think.

Also, what's the process to get such a design doc approved? Consensus on this thread?

Let's involve chromium-dev for best visibility. Let's ensure the owners of the flags agree and see if any one raises any breaking concerns.

 

On Wed, Jul 28, 2021 at 10:09 AM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1234043 to track the discussion/work.

On Wed, Jul 28, 2021 at 8:25 AM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

Will do. Thanks!

On Tue, Jul 27, 2021 at 8:25 PM Avi Drissman <a...@google.com> wrote:

I'd recommend coding it up as a proof of concept and once you have it working, float a proposal around.

On Tue, Jul 27, 2021 at 10:30 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

Thanks for the extra explanation!

So it seems BUILDFLAG(OS_WIN) would be the easiest and most consistent way then. It's only two letters more than defined(OS_WIN) so the length isn't too bad.

On Tue, Jul 27, 2021 at 7:00 PM Avi Drissman <a...@google.com> wrote:

My prototype was impossible to implement specifically because it was something like `OS(WIN)`. The problem was that a lot of the values inside the parentheses were already defined by the preprocessor and wrecked the token pasting games we were playing.

We need to have full control of the symbols we use, and that is what precluded my approach.

Avi

On Tue, Jul 27, 2021 at 9:10 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

In Avi's prototype, the plan is to introduce something like `OS(WIN)` instead of `defined(OS_WIN)`. I like the brevity of  `OS(WIN)` , but agree `BUILDFLAG(OS_WIN)` is more consistent, and is probably easier to implement. Does anyone have any preferences?

On Tue, Jul 27, 2021 at 1:34 PM Joe Mason <joenot...@google.com> wrote:

+1 to BUILDFLAG(OS_WIN). Reusing BUILDFLAG means fewer concepts to remember than having another similar macro.

(Unless there are semantic differences between OS_ macros and BUILDFLAG that I haven't thought of. Remembering how BUILDFLAG(OS_FOO) differs from BUILDFLAG(FOO) would be even worse.)

On Mon, Jul 26, 2021 at 11:11 PM 'Will Cassella' via cxx <c...@chromium.org> wrote:

Ah true. I'd be in favor of something like `BUILDFLAG(OS_WIN)` then, or `BUILD_OS_WIN()`

On Monday, July 26, 2021 at 7:34:58 PM UTC-7 dch...@chromium.org wrote:

The advantage to using a macro function is that it will fail to build if the correct header that defines the macro function is not included; without the macro function, the block would just silently be not compiled.

Daniel

On Mon, 26 Jul 2021 at 19:31, 'Will Cassella' via cxx <c...@chromium.org> wrote:

Would it have to be an invocated macro like that? Why not just something like `#if BUILD_OS_WIN`?

On Monday, July 26, 2021 at 1:16:43 PM UTC-7 Xiaohan Wang wrote:

Thanks for the context. I don't know what it entails but I am interested in giving it a try :)

On Mon, Jul 26, 2021 at 11:45 AM 'Avi Drissman' via cxx <c...@chromium.org> wrote:

As soon as I found that bare os strings, like XXXX(LINUX), wouldn't work, I abandoned it. If you want to propose a change, go for it, but there isn't anything here specifically to resume.

On Mon, Jul 26, 2021 at 2:36 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

The presubmit only applies to things prefixed with OS, my typo is like "OH_WIN" :)

I am glad I am not alone here. Is there a plan to resume this work? I wonder whether I can help.

On Mon, Jul 26, 2021 at 11:22 AM Avi Drissman <a...@google.com> wrote:

The CL in which I tried to make OS(xxxx) happen (equivalent to your proposal) was https://crrev.com/c/2288751. I don't have current memories of why it didn't work.

On Mon, Jul 26, 2021 at 2:16 PM Avi Drissman <a...@google.com> wrote:

We have a presubmit to try to catch those issues, so I'm curious why that didn't fire for you.

I was the last one to change build_config.h, and specifically tried to get BUILDFLAG working for OSes, and ran into fundamental blockers that prevented it from happening, though I can’t find my notes and the info is paged out of my head.

Avi

On Mon, Jul 26, 2021 at 2:06 PM Xiaohan Wang <xhw...@chromium.org> wrote:

Hello,

I had a typo in `defined(OS_WIN)` that wasn't caught during testing and code review. I wonder whether it makes sense to convert to use BUILDFLAGs so it's easier to catch similar issues, something like `BUILDFLAG(OS_WIN)`?

Thanks!

Best,

Xiaohan

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/37a04d37-bf51-4643-bc9d-7cc6efc6bbdfn%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAZ%3D1fyKOA9w7QJaBzFCfYXgFR4uhgbngSLES75Brw-bOg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/b9a6459f-fdc0-431c-8692-992e671405can%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/ca1ebabc-803a-43d1-9acf-8a73882bebfan%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF1j9YO5ohotHxjvm69igDwJqwPEno-cTSVTrC4ahafrEuDYqw%40mail.gmail.com.

--
--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQ5V6kpYOj0uKr0cQXH2_3uOfie3D3pTUawjhGgbK8sBA%40mail.gmail.com.

[Avi Drissman]

I saw that https://crbug.com/1234043 landed; congrats.

I'm still seeing a majority of OS_* includes across the source base. What's the status of that migration?

[Xiaohan Wang (王消寒)]

Sorry for not communicating on this more proactively. I was totally swamped by other projects so I didn't get a chance to finish the migration.

I did propose the migration to chrome code health rotation, but that has not been triaged yet.

+Nico Weber also commented in the design doc that this change might affect go/chrome-build-time. So we probably should measure the impact before mass-migration.

Then to avoid the manual work, it'd be great to write a script to automate (or at least partially) the migration. The complexity is on #include headers and gn deps, but should be doable.

If anyone is interested, you are more than welcome to contribute/help. Otherwise I'll probably pick up this work again later this year (during holiday season?).

Best,

Xiaohan

[Avi Drissman]

Good to hear it's still being worked on, though if there are genuine questions about whether it would regress build time, I'd imagine we'd want to get answers to those soon to know if migration would be worthwhile.

[Xiaohan Wang (王消寒)]

I finally took some time and made some progress here.

I wrote a script and did most of the migration locally, including replacing defined(OS_XXX) with BUILDFLAG(IS_XXX), add includes to "build/os_buildflags.h", remove includes to "build_config.h" if needed, and add GN dependency to "//build:os_buildflags". Then I did some performance testing. `gn gen` slowed by about 1% (from 9.89s to 10.05s). Actually build time (for both full build w/o goma and empty build) doesn't change much.

So I think the changes are good to go from a performance perspective. 

For completeness, there's another idea I discussed with thakis@, and I'd like to make sure it's covered here before we do the mass migration.

Currently `build_config.h` doesn't belong to any gn target. This confused me initially but now I can see it has at least two benefits:

1. Faster `gn gen` time. Since this header is included in many files, having a gn target would cause one additional entry in the .ninja file.
2. Developers don't need to worry about adding gn deps.

So one option is to extend this model, by defining the buildflags directly in os_buildflags.h and check in the file to build/, e.g.

#include "build/buildflag.h" 
#if defined(OS_WIN)
#define BUILDFLAG_INTERNAL_IS_WIN() (1)
#else
#define BUILDFLAG_INTERNAL_IS_WIN() (0)
#endif
......

This will share the same benefits as mentioned above. But we lose the benefits of having the same source of truth for both C++ and BUILD.gn checks. This is also a bit hacky and could confuse people... Thoughts?

Best,

Xiaohan

[Xiaohan Wang (王消寒)]

I forgot to mention, the detailed performance measurement process and result are documented here.

[Xiaohan Wang (王消寒)]

Update: Mass migration CLs are in review. I expect to land them over the next week or so assuming no issues.

[Xiaohan Wang (王消寒)]

The migration is done.

Build Performance:

Build performance doesn't seem to be affected (which is good):

https://commondatastorage.googleapis.com/chromium-browser-clang/build-time.html#from:2021-12-22

Summary:

- 386 CLs with +33599, -32531 lines

- New buildflags are directly defined in build/build_config.h so header update is minimal, and no BUILD.gn update needed.

- PRESUBMIT.py updated to prevent the use of the deprecated OS defines.

- Updated .js and .py files that generate C++ code.

- There might still be some defined(OS_XXX) left in third_party/ repos, which I'll leave to owners to decide.

Cheers,

Xiaohan

-----------------------------------

If you are still reading...

Miscellaneous:

It seems there's no convention on whether the comment on #else and #endif should be the same as the #if condition, or should be the reverse, e.g. should we have "!" or not? I was following the "same" condition.

```

#if  BUILDFLAG(IS_WIN)
#else // ! BUILDFLAG(IS_WIN)
#endif // !BUILDFLAG(IS_WIN)

```

Some people even go further with the reverse logic:

```

#if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS_LACROS)
#else
#endif  // !BUILDFLAG(IS_LINUX) && !BUILDFLAG(IS_CHROMEOS_LACROS)

```

clang format doesn't handle #endif comment well in the following case:

```

#if BUILDFLAG(IS_WIN)
#include <basetsd.h>  // Included before jpeglib.h because of INT32 clash
#endif                // BUILDFLAG(IS_WIN) 

```

Tricium likes to give the wrong suggestion for the following case:

"ClangTidy/modernize-use-equals-default"
use '= default' to define a trivial default constructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html)

```

Foo::Foo() {
#if BUILDFLAG(IS_MAC)
  Bar();
#endif
}

```

[Avi Drissman]

Excellent news; congratulations! 

I have a question about your last statement:

- There might still be some defined(OS_XXX) left in third_party/ repos, which I'll leave to owners to decide.

Are those OS_XXX macros ones that the third_party/ repos define themselves and then use, or are those third_party/ repos using OS_XXX macros that are defined by the top-level Chromium build/build_config.h?

If the former, I agree that migration should be left to those repo owners. If the latter, then we should migrate those, and I'm happy to assist, as I've done a lot of migration of third_party/ repos that depend on us. (I'm looking at you, Crashpad and libphonenumber!)

Is there anything we can do to clean up the OS_XXX macros from build/build_config.h?

Avi

 

[Nico Weber]

Re build_config.h: we mirror out //build into a separate git repo that other projects like v8, webrtc, etc deps in. So removing things from there requires all those projects to complete this too.

xhwang: hooray for doing this! OS checks look much nicer and more consistent with other buildflag checks, and misspelling an OS name is now a build error. Very cool :)

[Xiaohan Wang (王消寒)]

I forgot to mention: Huge thanks to @Nico Weber who reviewed almost all my CLs. He didn't just stamp them; he actually found many issues that I had to fix directly or fix my script. I feel it's probably easier for me to run the script to generate the CLs than him reviewing them :)

Avi: For third_party, I converted third_party/blink and third_party/widevine. @Mark Mentovai helped convert third_party/crashpad. It'll be great if you could help convert other folders under third_party.