Bug 221621 - [Cocoa] rename `ENGINEERING_BUILD` to `ENABLE_DEVELOPER_MODE` to match other platforms
Summary: [Cocoa] rename `ENGINEERING_BUILD` to `ENABLE_DEVELOPER_MODE` to match other ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-09 11:56 PST by Devin Rousso
Modified: 2021-02-10 14:28 PST (History)
17 users (show)

See Also:


Attachments
Patch (6.02 KB, patch)
2021-02-09 11:59 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-02-09 11:56:23 PST
see <https://bugs.webkit.org/show_bug.cgi?id=221371#c16>
Comment 1 Devin Rousso 2021-02-09 11:59:19 PST
Created attachment 419749 [details]
Patch
Comment 2 Alexey Proskuryakov 2021-02-09 12:21:55 PST
It's great to rename it from ENGINEERING_BUILD, but I am still concerned about introducing this concept. It's not OK to have different code between Release and Production. It's not OK to assume that local builds are Release, and CI builds are Production (they often aren't).
Comment 3 Devin Rousso 2021-02-09 14:38:39 PST
(In reply to Alexey Proskuryakov from comment #2)
> It's great to rename it from ENGINEERING_BUILD, but I am still concerned about introducing this concept. It's not OK to have different code between Release and Production. It's not OK to assume that local builds are Release, and CI builds are Production (they often aren't).

It sounds like you don't want this to land then?  IMO there's a huge amount of utility in being able to have minor tweaks that make debugging easier for local builds (e.g. giving a name to the injected media controls scripts so it's visible in Web Inspector).  I'm not trying to "shame" you or change your mind or anything like that, I'm just asking because if this isn't going to land then we should land Bug 221613 in the meantime (or perhaps remove `ENGINEERING_BUILD` altogether) so non-Xcode builds aren't blocked.

FYI WebInspectorUI has been doing something similar to this for years with `COMBINE_INSPECTOR_RESOURCES`/`COMBINE_TEST_RESOURCES` and AFAIK there haven't been any issues there.
Comment 4 Devin Rousso 2021-02-09 14:51:51 PST
Comment on attachment 419749 [details]
Patch

Seeing as `ENGINEERING_BUILD` is already in trunk and this patch just renames it so that non-Xcode builds aren't broken, I'm going to land this.

I'm not doing this as any sort of "put my foot down and say we're keeping it" kind of thing. I'm more than willing to revisit the discussion (and even remove it if that's the agreed upon result).  I just want to unblock non-Xcode builds.
Comment 5 EWS 2021-02-09 15:06:58 PST
Committed r272617: <https://commits.webkit.org/r272617>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419749 [details].
Comment 6 Alexey Proskuryakov 2021-02-09 15:07:13 PST
We should remove ENGINEERING_BUILD altogether, and find a different way to achieve the goal. It's not doing quite what you want it to do, and it's going to get farther away from what's needed in the future.
Comment 7 Michael Catanzaro 2021-02-09 15:11:50 PST
I don't think it will ever be possible to remove DEVELOPER_MODE from other ports.

E.g. it's used in CMake to decide whether to hide internal symbols, which can't be done in development builds since that would prevent linking API tests, layout test runner, etc.
Comment 8 Alexey Proskuryakov 2021-02-09 19:30:31 PST
I am not concerned about other ports. It is the invalid distinction between Debug/Release and production builds on Apple platforms that should not exist. 

Devin, I’m going to post a patch to remove it tomorrow if you don’t beat me to it. We can think about better ways to achieve the goal separately.
Comment 9 Radar WebKit Bug Importer 2021-02-10 14:28:35 PST
<rdar://problem/74206419>