Bug 238556 – Turn DEVELOPER_MODE ON for all non-Apple ports in build-webkit

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Don Olmstead]

PlayStation isn't building with it.

[Don Olmstead]

Created attachment 456146 [details]
Patch

[Don Olmstead]

Comment on attachment 456146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456146&action=review

> Tools/Scripts/webkitdirs.pm:2565
>      # Some ports have production mode, but build-webkit should always use developer mode.
> -    push @args, "-DDEVELOPER_MODE=ON" if isGtk() || isJSCOnly() || isWPE() || isWin64();
> +    push @args, "-DDEVELOPER_MODE=ON" unless isAppleWebKit();

Originally this seemed to want to be turned ON for non-AppleWin ports. I'm really not sure if this should still be the case. I'm leaning towards it being on for all CMake ports but wanted to get an opinion from Apple before modifying it.

[Alex Christensen]

Comment on attachment 456146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456146&action=review

>> Tools/Scripts/webkitdirs.pm:2565
>> +    push @args, "-DDEVELOPER_MODE=ON" unless isAppleWebKit();
> 
> Originally this seemed to want to be turned ON for non-AppleWin ports. I'm really not sure if this should still be the case. I'm leaning towards it being on for all CMake ports but wanted to get an opinion from Apple before modifying it.

No strong opinion here.  This seems fine.  I wonder if we could work to remove DEVELOPER_MODE

[Don Olmstead]

Comment on attachment 456146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456146&action=review

>>> Tools/Scripts/webkitdirs.pm:2565
>>> +    push @args, "-DDEVELOPER_MODE=ON" unless isAppleWebKit();
>> 
>> Originally this seemed to want to be turned ON for non-AppleWin ports. I'm really not sure if this should still be the case. I'm leaning towards it being on for all CMake ports but wanted to get an opinion from Apple before modifying it.
> 
> No strong opinion here.  This seems fine.  I wonder if we could work to remove DEVELOPER_MODE

If there's no historical reason for not turning `DEVELOPER_MODE` `ON` for AppleWin then I think we should just turn it `ON` for all here so the above comment is actually true.

We could possibly roll `DEVELOPER_MODE` and `ENABLE_EXPERIMENTAL_FEATURES` together. For Igalia ports `DEVELOPER_MODE=OFF` seems to signal a production build for them. I'm not sure if you'd ever want to do `DEVELOPER_MODE=OFF` and `ENABLE_EXPERIMENTAL_FEATURES=ON`. I would guess there would need to be a transition period with that though.

[Alex Christensen]

I think we should be working towards removing DEVELOPER_MODE and ENABLE_EXPERIMENTAL_FEATURES and having development be more like production.

[Don Olmstead]

(In reply to Alex Christensen from comment #5)
> I think we should be working towards removing DEVELOPER_MODE and
> ENABLE_EXPERIMENTAL_FEATURES and having development be more like production.

My understanding is the Igalia releases use DEVELOPER_MODE=OFF. In those cases it doesn't build any testing bits or the MiniBrowser. Then they make their browser off of that.

So I'll propose we add in the DEVELOPER_MODE=ON for all ports. Then get the PlayStation port on that, which is my main concern. Then we can talk to Igalians about their release and hopefully merge ENABLE_EXPERIMENTAL_FEATURES into DEVELOPER_MODE. Then we can figure out if there's a way to get us off DEVELOPER_MDOE completely.

I'm not sure everyone's requirements for a production build so it might need to stick around completely. For our builds I'd like to minimize the number of functions exported and do a full LTO.

[Don Olmstead]

Also from grepping the code I do see uses of ENABLE(DEVELOPER_MODE) and ENABLE(EXPERIMENTAL_FEATURES).

ENABLE(DEVELOPER_MODE) appears in a number of .h/.cpp files. ENABLE(EXPERIMENTAL_FEATURES) only appears in a single .cpp file BUT is used in two WebPrefences yaml files.

[Alex Christensen]

EXPERIMENTAL_FEATURES is on in SafariTechnologyPreview and off everywhere else in Apple's ecosystem.  We discussed it a few months ago and decided it is better to have SafariTechnologyPreview more similar to everything else.

The more differences between DEVELOPER_MODE=ON and DEVELOPER_MODE=OFF there are, the more likely it is that a bug in some obscure configuration will go unfixed because no developers noticed it.

[Don Olmstead]

Created attachment 456159 [details]
Patch enabling all

[Michael Catanzaro]

We'll never be able to get rid of DEVELOPER_MODE because some things just need to be different for developer builds vs. real builds. E.g. if you're a developer you want to load the injected bundle from your build directory instead of the install prefix, otherwise there's no way it will work. But in releases we need to load the installed injected bundle. Another example: in developer builds we need to mount the build directory in the sandbox. For a more recent example, in bug #237107 where clopez needs to run a suboptimal codepath for WPT tests that we don't want to run in production.

Some cases look suspicious, though. E.g. I see we have Xvfb-related code guarded by DEVELOPER_MODE. I doubt those guards are truly needed.

Removing EXPERIMENTAL_FEATURES is probably possible: that's just a matter of replacing build guards with runtime guards.

[Alex Christensen]

Apple developers use DYLD_FRAMEWORK_PATH so habitually and it works so well that I think we don't want ENABLE(DEVELOPER_MODE) on Apple platforms.  I understand if it might be useful for linux developers, but we should keep it minimal.

[Alex Christensen]

I'm removing EXPERIMENTAL_FEATURES in https://bugs.webkit.org/show_bug.cgi?id=238565 and would appreciate some input from Linux devs because I'm not familiar with its use on Linux.

[Don Olmstead]

(In reply to Alex Christensen from comment #12)
> I'm removing EXPERIMENTAL_FEATURES in
> https://bugs.webkit.org/show_bug.cgi?id=238565 and would appreciate some
> input from Linux devs because I'm not familiar with its use on Linux.

Ok if that's the plan can we get a review on this?

[EWS]

Committed r292137 (249044@main): <https://commits.webkit.org/249044@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456146 [details].

[Radar WebKit Bug Importer]

<rdar://problem/91080551>