Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Keith Miller]

> On Aug 5, 2017, at 1:48 PM, Konstantin Tokarev  wrote:
> 04.08.2017, 21:20, "Carlos Alberto Lopez Perez" :
>> On 02/08/17 12:14, Konstantin Tokarev wrote:
>>>  FWIW, I use ENABLE_ALLINONE_BUILD=ON as a default build option in Qt
>>>  port and don't have any "terrible" development experience. Even when I
>>>  need to make a change in file that is not port-specific, usually just
>>>  one of AllInOne files needs to be recompiled
>> 
>> Do you happen to have some numbers of how much less time it takes to
>> build with this?
> 
> You can find old numbers here:
> 
> https://bugs.webkit.org/show_bug.cgi?id=102647#c11
> 

Right, I’m not proposing an all-in-one build; I agree that’s likely to be 
terrible. Rather this would be bundles of ~8 (still up for experiment) cpp 
files, which are automagically generated. These bundles would also be similar 
cpp files (i.e. from the same directory), so they are likely to include the 
same headers.

>> 
>> ,
>> 
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 
> -- 
> Regards,
> Konstantin
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Konstantin Tokarev]

04.08.2017, 21:20, "Carlos Alberto Lopez Perez" :
> On 02/08/17 12:14, Konstantin Tokarev wrote:
>>  FWIW, I use ENABLE_ALLINONE_BUILD=ON as a default build option in Qt
>>  port and don't have any "terrible" development experience. Even when I
>>  need to make a change in file that is not port-specific, usually just
>>  one of AllInOne files needs to be recompiled
>
> Do you happen to have some numbers of how much less time it takes to
> build with this?

You can find old numbers here:

https://bugs.webkit.org/show_bug.cgi?id=102647#c11

>
> ,
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Carlos Alberto Lopez Perez]

On 02/08/17 12:14, Konstantin Tokarev wrote:
> FWIW, I use ENABLE_ALLINONE_BUILD=ON as a default build option in Qt
> port and don't have any "terrible" development experience. Even when I
> need to make a change in file that is not port-specific, usually just
> one of AllInOne files needs to be recompiled

Do you happen to have some numbers of how much less time it takes to
build with this?



signature.asc
Description: OpenPGP digital signature
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Konstantin Tokarev]

03.08.2017, 01:07, "Michael Catanzaro" :
> On Wed, Aug 2, 2017 at 11:14 AM, Konstantin Tokarev  wrote:
>> FWIW, I use ENABLE_ALLINONE_BUILD=ON as a default build option in Qt port 
>> and don't have any "terrible" development experience. Even when I need to 
>> make a change in file that is not port-specific, usually just one of 
>> AllInOne files needs to be recompiled
>
> What... what does that mean? How is it all in one if there are multiple files?

There are large groups of files from WebCore which are compiled as one file for 
each group. Look for *AllInOne.cpp files in WebCore

-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Michael Catanzaro]

On Wed, Aug 2, 2017 at 11:14 AM, Konstantin Tokarev  
wrote:
FWIW, I use ENABLE_ALLINONE_BUILD=ON as a default build option in Qt 
port and don't have any "terrible" development experience. Even when 
I need to make a change in file that is not port-specific, usually 
just one of AllInOne files needs to be recompiled


What... what does that mean? How is it all in one if there are multiple 
files?
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Konstantin Tokarev]

02.08.2017, 01:49, "Michael Catanzaro" :
> On Tue, Aug 1, 2017 at 11:33 PM, Keith Miller  wrote:
>> P.S. There is also a reasonable chance that we will do some form of unified 
>> sources (compiling multiple cpp files at the same time). In that case we 
>> don’t need to change our config.h rules.
>
> Do you have experience with unified source builds on macOS? We basically 
> never do these on Linux, but it's of course possible. These builds are 
> typically great for production but terrible for development, since everything 
> needs to be recompiled when any file is changed. Also, using static to mark 
> functions as file-private no longer works. This is sure to cause headache. 
> But the benefits may be worthwhile.

FWIW, I use ENABLE_ALLINONE_BUILD=ON as a default build option in Qt port and 
don't have any "terrible" development experience. Even when I need to make a 
change in file that is not port-specific, usually just one of AllInOne files 
needs to be recompiled

Of course this may not be good for people actively developing Web-facing code

>
> Some good description:
>
> http://mesonbuild.com/Unity-builds.html
>
> Michael
> ,
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev


-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Keith Miller]

I’ve done some experiments with automagically building the unified source 
files. I have some data that I’ll share with the rest of WebKit when I have 
more information. But as a quick note, since my current approach to unified 
sources has the build system decide which cpp files to bundle together, the 
number of cpp files in the same translation unit will probably be if you think 
you will likely be doing a lot of incremental builds.

I agree that there are definitely a number of downsides to unified sources. 
Ultimately, my primary goal is to make the developer experience as smooth as 
possible, I’m less concerned with production build times. Although, I would 
like to improve production build times also.

Cheers,
Keith

> On Aug 1, 2017, at 3:48 PM, Michael Catanzaro  wrote:
> 
> On Tue, Aug 1, 2017 at 11:33 PM, Keith Miller  wrote:
>> P.S. There is also a reasonable chance that we will do some form of unified 
>> sources (compiling multiple cpp files at the same time). In that case we 
>> don’t need to change our config.h rules.
> 
> Do you have experience with unified source builds on macOS? We basically 
> never do these on Linux, but it's of course possible. These builds are 
> typically great for production but terrible for development, since everything 
> needs to be recompiled when any file is changed. Also, using static to mark 
> functions as file-private no longer works. This is sure to cause headache. 
> But the benefits may be worthwhile.
> 
> Some good description:
> 
> http://mesonbuild.com/Unity-builds.html 
> 
> 
> Michael

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Michael Catanzaro]

On Tue, Aug 1, 2017 at 11:33 PM, Keith Miller  
wrote:
P.S. There is also a reasonable chance that we will do some form of 
unified sources (compiling multiple cpp files at the same time). In 
that case we don’t need to change our config.h rules.


Do you have experience with unified source builds on macOS? We 
basically never do these on Linux, but it's of course possible. These 
builds are typically great for production but terrible for development, 
since everything needs to be recompiled when any file is changed. Also, 
using static to mark functions as file-private no longer works. This is 
sure to cause headache. But the benefits may be worthwhile.


Some good description:

http://mesonbuild.com/Unity-builds.html

Michael
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Keith Miller]

That’s right if we end up going with C++ modules. We will either need to 
produce a replacement for config.h or have all root headers (i.e. headers that 
don’t include any other WebKit headers) include config.h. This is because with 
C++ modules the rule is that you have to include what you use. C++ modules 
basically precompile all the ASTs of your headers, which is not possible to do 
without all your macro/configuration definitions. 

I’m not sure there is a great replacement for a configuration header but 
perhaps we can come up with one. 

Cheers,
Keith

P.S. There is also a reasonable chance that we will do some form of unified 
sources (compiling multiple cpp files at the same time). In that case we don’t 
need to change our config.h rules.

> On Aug 1, 2017, at 2:12 PM, Ryosuke Niwa  wrote:
> 
> On Mon, Jul 31, 2017 at 1:27 PM, Darin Adler  wrote:
>> We originally adopted this “config.h” style to make WebKit buildable with 
>> autotools. Since that has not been a consideration for years I would be 
>> willing to abandon this and change how we do things.
>> 
>> I don’t think we should add lots of includes of “config.h”, though. I think 
>> we can come up with something better.
> 
> As I understand it, we need to change the way we include config.h to
> enable C++ module in WebKit as well since each header file needs to be
> able to compile as its own module.
> 
> - R. Niwa

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Ryosuke Niwa]

On Mon, Jul 31, 2017 at 1:27 PM, Darin Adler  wrote:
> We originally adopted this “config.h” style to make WebKit buildable with 
> autotools. Since that has not been a consideration for years I would be 
> willing to abandon this and change how we do things.
>
> I don’t think we should add lots of includes of “config.h”, though. I think 
> we can come up with something better.

As I understand it, we need to change the way we include config.h to
enable C++ module in WebKit as well since each header file needs to be
able to compile as its own module.

- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Alicia Boya García]

On 08/01/2017 08:25 AM, Darin Adler wrote:
>> On Jul 31, 2017, at 2:04 PM, Michael Catanzaro  wrote:
>>
>> On Mon, Jul 31, 2017 at 9:27 PM, Darin Adler  wrote:
>>> I don’t think we should add lots of includes of “config.h”, though. I think 
>>> we can come up with something better.
>>
>> Like what?
> 
> We should consider reducing the size of “config.h” and only use it for things 
> that it’s really needed for, using more conventional header includes for 
> other purposes.
> 
> Ideally we would eliminate “config.h” entirely and replace it with normal 
> headers.
> 
> I definitely don’t think we need to use the same name, “config.h”, for 9 
> separate headers that don’t have the same contents in 9 different directories 
> in the overall WebKit source code. A risk of putting includes of “config.h” 
> in header files is that it isn’t clear which of the “config.h” files will be 
> included. If we aren’t obliged to use the name “config.h” because of 
> autotools, then perhaps we can name these more in line with WebKit coding 
> style, something like WebCoreConfiguration.h, but also strive to get rid of 
> them entirely.
> 
> In projects not using autotools and using IDEs to build I often see a header 
> referred to as the “prefix” that is automatically included by the IDE, with 
> no #include statements at all in the source files. That’s used as a sort of 
> substitute for #define statements on the command line and is what I used 
> before I became familiar with autotools.
> 
> I am concerned to see there is also a header named “cmakeconfig.h” so maybe 
> we do need “config.h”.
> 
> — Darin
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 

I'm not sure if it's realistic to get rid of configuration headers.
WebKit development builds use just too many build-time flags to enable
features.

On the other hand, I agree completely in the name issue, which is also
an scoping issue. The situation I found is something like this:

"cmakeconfig.h" is a file generated by CMake with all the flags we
enable at configure time. One reason to do that is precisely because
there are too many to put comfortably in the command line (I count 177
#define's there).

There is a single cmakeconfig.h file for an entire WebKit build. This
makes sense: If a feature is implemented in both WebCore and WebKit you
want it to be enabled either both or in none.

On the other hand, there is currently a config.h file in every of our
sub-project directories: WTF, WebCore, JavaScriptCore, WebKit and
WebDriver. Every config.h file includes cmakeconfig.h, plus some other
stuff. Unfortunately, the kind of "stuff" that is additionally included
or defined varies a lot between sub-projects.

Having several files called config.h is confusing at the very least,
even more when they are included from different sub-projects. The same
#include "config.h" line refers to different files in different contexts.

This is a quite nasty problem, as the author of a header file in a
sub-project may assume it's safe to rely on something being defined
because it's included in that sub-project's config.h file... and most of
the time it will work, but it may not be the case when that file is
included (even transitively) by any other file from other sub-project
that uses a different config.h file.

As a consequence, quirks often have to be added to config.h files of
projects with dependencies. For instance, JavaScriptCore/config.h
includes "runtime/JSExportMacros.h" (and therefore many headers in JSC
assume it is included) and the same happens in WebCore, whose config.h
includes "WebCore/PlatformExportMacros.h". The WebKit sub-project
includes headers from both, so its config.h has to include both
"runtime/JSExportMacros.h" and "WebCore/PlatformExport.h" too in order
to build.

Complicating things a bit more there is
ThirdParty/gtest/include/gtest/internal/gtest-port.h. In the gtest
compilation context there is no config.h in path, but it uses WTF and
JSC files nevertheless, and some of these actually check for
configuration flags:

#include 
#include 
#include 
#include 
#include 



There are a few things all config.h files agree:

* Include cmakeconfig.h if a CMake build is being used.
* Include wtf/FastMalloc.h.
* Include wtf/DisallowCType.h.

Other than that, the differences are mostly sub-project specific quirks
to get around compilation issues of the sub-project. The case of the
WebKit sub-project is a bit more worrying as it has many defaults for
flags there, but they would not be read from the context of other
sub-projects.

We should consider why we have several config.h files and if that makes
sense or there are better alternatives. In principle, since config.h is
about configuration flags, and there is a single global set of flags for
the entire project, it would make much more sense to have a single true
config.h instead of sub-project-specific ones.

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Alicia Boya García]

On 07/31/2017 11:16 PM, Maciej Stachowiak wrote:
> 
> 
>> On Jul 31, 2017, at 5:04 PM, Michael Catanzaro  wrote:
>>
>> On Mon, Jul 31, 2017 at 9:27 PM, Darin Adler  wrote:
>>> I don’t think we should add lots of includes of “config.h”, though. I think 
>>> we can come up with something better.
>>
>> Like what? The only alternative is to pass defines as preprocessor flags via 
>> -D. Our command lines are already hard enough to read as it is. Or, well, we 
>> could include config.h in Platform.h, where the ENABLE/USE series of macros 
>> are defined.
>>
>> I'm curious as to why including config.h in the header file would break 
>> Autotools builds. I don't see why that would be the case. As far as I know, 
>> it just happens to be common practice to include config.h only in source 
>> files. It's special: you always include it first in source files to avoid 
>> extremely confusing bugs that can result from not doing so, and therefore it 
>> seems redundant to put it in header files.
>>
>> I wonder how other projects (and other IDEs, e.g. XCode) handle this 
>> situation. Anyway, I don't have a strong opinion either way
> 
> I think "config.h" as the first body include, and never in headers, is mostly 
> just a stylistic choice in projects that use autotools.
> 
> However, here are some specific issues to consider:
> (1) Public API headers (or installed private interfaces) should not include 
> config.h, and should not rely on any macros defined there.
> (2) It's probably ok to include it in non-API headers, at least any that use 
> macros defined by config.h.
> (3) Implementation files may need to still include config.h. They might not 
> include any headers that include config.h, or they might currently but don't 
> want to rely on it. config.h may also define macros that affect the behavior 
> of system headers and therefore should be included before any of them, so it 
> needs to go first.
> (4) Given combo of (2) and (3), some includes of config.h in headers may be 
> redundant. I am not sure if this causes any practical problems though.
> 
>  - Maciej
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 

Regarding (1) and (2), the patch I'm proposing only modifies files that
use either USE() or ENABLED() macros. Since public API headers should
not depend on any compilation flags, they should not use either of these
and therefore they will not be modified by the patch.

I agree with (3). All WebKit .cpp files should still include "config.h".

Regarding (4), there should not be any problems as long as the config.h
files are guarded (either by #ifndef or #pragma once). Any effect on
build times should be minimal as the compilers resolve these guards very
quickly (and so far this has been true in my limited tests).

— Alicia.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Darin Adler]

> On Jul 31, 2017, at 2:04 PM, Michael Catanzaro  wrote:
> 
> On Mon, Jul 31, 2017 at 9:27 PM, Darin Adler  wrote:
>> I don’t think we should add lots of includes of “config.h”, though. I think 
>> we can come up with something better.
> 
> Like what?

We should consider reducing the size of “config.h” and only use it for things 
that it’s really needed for, using more conventional header includes for other 
purposes.

Ideally we would eliminate “config.h” entirely and replace it with normal 
headers.

I definitely don’t think we need to use the same name, “config.h”, for 9 
separate headers that don’t have the same contents in 9 different directories 
in the overall WebKit source code. A risk of putting includes of “config.h” in 
header files is that it isn’t clear which of the “config.h” files will be 
included. If we aren’t obliged to use the name “config.h” because of autotools, 
then perhaps we can name these more in line with WebKit coding style, something 
like WebCoreConfiguration.h, but also strive to get rid of them entirely.

In projects not using autotools and using IDEs to build I often see a header 
referred to as the “prefix” that is automatically included by the IDE, with no 
#include statements at all in the source files. That’s used as a sort of 
substitute for #define statements on the command line and is what I used before 
I became familiar with autotools.

I am concerned to see there is also a header named “cmakeconfig.h” so maybe we 
do need “config.h”.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Maciej Stachowiak]

> On Jul 31, 2017, at 5:04 PM, Michael Catanzaro  wrote:
> 
> On Mon, Jul 31, 2017 at 9:27 PM, Darin Adler  wrote:
>> I don’t think we should add lots of includes of “config.h”, though. I think 
>> we can come up with something better.
> 
> Like what? The only alternative is to pass defines as preprocessor flags via 
> -D. Our command lines are already hard enough to read as it is. Or, well, we 
> could include config.h in Platform.h, where the ENABLE/USE series of macros 
> are defined.
> 
> I'm curious as to why including config.h in the header file would break 
> Autotools builds. I don't see why that would be the case. As far as I know, 
> it just happens to be common practice to include config.h only in source 
> files. It's special: you always include it first in source files to avoid 
> extremely confusing bugs that can result from not doing so, and therefore it 
> seems redundant to put it in header files.
> 
> I wonder how other projects (and other IDEs, e.g. XCode) handle this 
> situation. Anyway, I don't have a strong opinion either way

I think "config.h" as the first body include, and never in headers, is mostly 
just a stylistic choice in projects that use autotools.

However, here are some specific issues to consider:
(1) Public API headers (or installed private interfaces) should not include 
config.h, and should not rely on any macros defined there.
(2) It's probably ok to include it in non-API headers, at least any that use 
macros defined by config.h.
(3) Implementation files may need to still include config.h. They might not 
include any headers that include config.h, or they might currently but don't 
want to rely on it. config.h may also define macros that affect the behavior of 
system headers and therefore should be included before any of them, so it needs 
to go first.
(4) Given combo of (2) and (3), some includes of config.h in headers may be 
redundant. I am not sure if this causes any practical problems though.

 - Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Michael Catanzaro]

On Mon, Jul 31, 2017 at 9:27 PM, Darin Adler  wrote:
I don’t think we should add lots of includes of “config.h”, 
though. I think we can come up with something better.


Like what? The only alternative is to pass defines as preprocessor 
flags via -D. Our command lines are already hard enough to read as it 
is. Or, well, we could include config.h in Platform.h, where the 
ENABLE/USE series of macros are defined.


I'm curious as to why including config.h in the header file would break 
Autotools builds. I don't see why that would be the case. As far as I 
know, it just happens to be common practice to include config.h only in 
source files. It's special: you always include it first in source files 
to avoid extremely confusing bugs that can result from not doing so, 
and therefore it seems redundant to put it in header files.


I wonder how other projects (and other IDEs, e.g. XCode) handle this 
situation. Anyway, I don't have a strong opinion either way


Michael
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] What's the rationale for not including config.h in any header files?

[Darin Adler]

We originally adopted this “config.h” style to make WebKit buildable with 
autotools. Since that has not been a consideration for years I would be willing 
to abandon this and change how we do things.

I don’t think we should add lots of includes of “config.h”, though. I think we 
can come up with something better.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev