| Summary: | [Cocoa] rename `ENGINEERING_BUILD` to `ENABLE_DEVELOPER_MODE` to match other platforms | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||
| Component: | New Bugs | Assignee: | Devin Rousso <hi> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | ap, bburg, calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hi, jer.noble, joepeck, mcatanzaro, philipj, sergio, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=221613 https://bugs.webkit.org/show_bug.cgi?id=221371 https://bugs.webkit.org/show_bug.cgi?id=221684 |
||||||
| Attachments: |
|
||||||
|
Description
Devin Rousso
2021-02-09 11:56:23 PST
Created attachment 419749 [details]
Patch
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). (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 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.
Committed r272617: <https://commits.webkit.org/r272617> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419749 [details]. 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. 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. 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. |