search

ungoogled-software / ungoogled-chromium

Google Chromium, sans integration with Google
BSD 3-Clause "New" or "Revised" License
20.8k stars 844 forks source link

Fix builds with DCHECK enabled #3009

Closed skrinakron closed 2 months ago

skrinakron commented 2 months ago

Removes stranded uses of preferences members patched out elsewhere. As the only occurrences appear to be in portions compiled only when DCHECK is enabled, this allows full debug builds to now succeed. Fixes #3008.

PF4Public commented 2 months ago

Interesting, the whole patch fails to apply.

Ahrotahn commented 2 months ago

The offsets just need to be updated since there's one fewer line now:

diff ```diff --- a/patches/core/ungoogled-chromium/remove-unused-preferences-fields.patch +++ b/patches/core/ungoogled-chromium/remove-unused-preferences-fields.patch @@ -6571,7 +6571,7 @@ DCHECK(pref_account_id.empty() || !consented_to_sync || pref_account_id == account_info.account_id.ToString()) -@@ -545,10 +417,6 @@ void PrimaryAccountManager::SetSyncPrima +@@ -545,10 +416,6 @@ void PrimaryAccountManager::SetSyncPrima // Go ahead and update the last signed in account info here as well. Once a // user is signed in the corresponding preferences should match. Doing it here // as opposed to on signin allows us to catch the upgrade scenario. @@ -6582,7 +6582,7 @@ } void PrimaryAccountManager::SetPrimaryAccountInternal( -@@ -560,22 +428,6 @@ void PrimaryAccountManager::SetPrimaryAc +@@ -560,22 +427,6 @@ void PrimaryAccountManager::SetPrimaryAc // 'account_info' might be a reference to the contents of `primary_account_`. // Create a PrimaryAccount object before calling emplace to avoid crashes. primary_account_.emplace(PrimaryAccount(account_info, consented_to_sync)); @@ -6605,7 +6605,7 @@ } void PrimaryAccountManager::RecordHadPreviousSyncAccount() const { -@@ -585,7 +437,7 @@ void PrimaryAccountManager::RecordHadPre +@@ -585,7 +436,7 @@ void PrimaryAccountManager::RecordHadPre } const std::string& last_gaia_id_with_sync_enabled = @@ -6614,7 +6614,7 @@ const bool existed_primary_account_with_sync = !last_gaia_id_with_sync_enabled.empty(); -@@ -735,38 +587,6 @@ PrimaryAccountChangeEvent::State Primary +@@ -735,38 +586,6 @@ PrimaryAccountChangeEvent::State Primary void PrimaryAccountManager::ComputeExplicitBrowserSignin( const PrimaryAccountChangeEvent& event_details, ScopedPrefCommit& scoped_pref_commit) { @@ -6653,7 +6653,7 @@ } void PrimaryAccountManager::FirePrimaryAccountChanged( -@@ -850,7 +670,6 @@ void PrimaryAccountManager::OnSigninAllo +@@ -850,7 +669,6 @@ void PrimaryAccountManager::OnSigninAllo bool PrimaryAccountManager::ShouldSigninAllowedPrefAffectPrimaryAccount( bool is_sync_consent) { return switches::IsExplicitBrowserSigninUIOnDesktopEnabled() && ```
skrinakron commented 2 months ago

Sorry and thanks for the correction. I did wonder about the line offset but got a false sense of security when patch gave neither errors nor rejections. Using verify_patches.py I now see the errors as well. I've amended the commit with the above changes.

Do you happen to know if there's a way to make patch as fastidious about matches as verify_patches.py is? I've tried the options -N -F0 --posix --verbose -r /known/path/rejects_file, and yet the faulty patch keeps applying without so much as a warning.

Ahrotahn commented 2 months ago

I'm not aware of any way to make patch fail when encountering offset differences, but you could instead use git apply as a replacement which will fail if the patch can't be applied cleanly (it also defaults to -p1).

Granted most of our patches wouldn't apply cleanly without many of the of previous patches in the series already applied. You would want to use quilt to make refreshing the patches for this repo easier.