Remove chrome-installer-util-util_constants.cc.patch #8063
Conversation
This patch can be removed in favour of chromium_src overrides to redefine the values for the constants we're interested in. Resolves brave/brave-browser#14340
mariospr
force-pushed
the
issues/14340
branch
from
7144423 to
0ef49fc
Compare
|
|
||
| namespace env_vars { | ||
|
|
||
| #if defined(OFFICIAL_BUILD) |
There was a problem hiding this comment.
I think we can safely delete this guard because we only support installer build for official release.
Using kGoogleUpdateIsMachineEnvVar_ChromiumImpl below doesn't have much meaning.
There was a problem hiding this comment.
I've talked to @simonhong offline to better understand this and we agreed on that removing that OFFICIAL_BUILD guard would make sense in that it's not actually used for the installer, but that would divert a bit from the conversion we're trying to do here, as that would slightly change the logic originally implemented in the patch we're removing, which is considering official and non-official builds.
Therefore, we agreed to keep this bit in this PR and then work separately on another PR not just to remove that guard from this override, but to also do it from other overrides and patches affecting //chrome/installer/** files in general, so that we can cleanup and simplify things further in a more consistent way.
There was a problem hiding this comment.
New issue filed to track down that simplification effort: brave/brave-browser#14364
While landing PR #8063 [1], we didn't realize that the assignment in the chromium_src override was wrong because the CI bots didn't complain, since the installer is not supported in non official builds, and because I normally build on Linux only. Then I filed issue 14365 [2] to cleanup all those non-official code paths from changes to //chrome/installer/** files and, as soon as I started building on my Windows machine with official_build = false I hit a build error because of this assignment. Therefore, while the long term fix will come as part of fixing that other issue, it would be great to get non-official Windows building again. [1] https://github.com/brave/brave-core/pull/8063/files [2] brave/brave-browser#14364 Resolves brave/brave-browser#14365
This patch can be removed in favour of chromium_src overrides to
redefine the values for the constants we're interested in.
Resolves brave/brave-browser#14340
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
N/A (it relates to #109 but there's no test plan there)