Remove dl.google.com repository from Linux packages #1078
Conversation
|
I'm not sure how to build the So this is currently untested. |
|
@w0ts0n Who would be a good reviewer for this? |
|
@bbondy maybe? Or he can triage appropriately. |
|
I think either @mbacchi or @mihaiplesa. Added them. |
|
If one of you can tell me how the packages are built, I wouldn't mind testing this locally before you review it. |
Can you tell us how the Unlike Mac and Windows, building the Brave rpm/deb files on Linux does not require any special signing steps, so you should be able to build it without anything special setup. Otherwise we'll try to test this PR in a build as time allows. |
|
Oh, you may want to check the wiki instructions here: https://github.com/brave/brave-browser/wiki It describes Let me know if that helps. |
| # Setup the installation directory hierachy in the package staging area. | ||
| prep_staging_rpm() { | ||
| prep_staging_common | ||
| - install -m 755 -d "${STAGEDIR}/etc/cron.daily" |
There was a problem hiding this comment.
By not running this install command, I get an error that the staging directory doesn't exist:
[74/93] ACTION //chrome/installer/linux:unstable_rpm(//build/toolchain/linux:clang_x64)
FAILED: chromium-browser-dev-72.0.61.0-1.x86_64.rpm
python ../../build/gn_run_binary.py installer/rpm/build.sh -a x64 -b . -c unstable -d brave -o . -t linux -f
fakeroot rpmbuild -bb --target=x86_64 --rmspec --define _topdir /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-build-dev --define _binary_payload w9.xzdio --define __os_install_post %{nil} /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-tmp-dev/chrome.spec
Building target platforms: x86_64
Building for target x86_64
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.bmNUOq
+ umask 022
+ cd /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-build-dev/BUILD
+ exit 0
Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.bMUPCt
+ umask 022
+ cd /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-build-dev/BUILD
+ rm -rf /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-build-dev/BUILDROOT/brave-browser-dev-0.61.0-1.x86_64
+ [ -z /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-staging-dev -o ! -d /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-staging-dev ]
+ [ -z /opt/brave.com/brave-dev -o ! -d /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-staging-dev//opt/brave.com/brave-dev ]
+ install -m 755 -d /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-build-dev/BUILDROOT/brave-browser-dev-0.61.0-1.x86_64/etc /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-build-dev/BUILDROOT/brave-browser-dev-0.61.0-1.x86_64/opt /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-build-dev/BUILDROOT/brave-browser-dev-0.61.0-1.x86_64/usr
+ cp -a /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-staging-dev/etc/ /var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-build-dev/BUILDROOT/brave-browser-dev-0.61.0-1.x86_64/
cp: cannot stat '/var/lib/jenkins/workspace/brave-browser/src/out/Release/rpm-staging-dev/etc/': No such file or directory
error: Bad exit status from /var/tmp/rpm-tmp.bMUPCt (%install)
Both this and the below template processing may be necessary, but we should not include the cron.daily script via other means. By adding this back in, and removing the cron.daily I can get the create_dist step to complete, but think that is a rather inelegant solution.
| log_cmd echo "Staging RPM install files in '${STAGEDIR}'..." | ||
| - process_template "${BUILDDIR}/installer/common/rpmrepo.cron" \ | ||
| - "${STAGEDIR}/etc/cron.daily/${PACKAGE}" | ||
| - chmod 755 "${STAGEDIR}/etc/cron.daily/${PACKAGE}" |
There was a problem hiding this comment.
This patch builds the rpm, but it might not work as desired, I'll test that shortly:
diff --git a/patches/chrome-installer-linux-rpm-build.sh.patch b/patches/chrome-installer-linux-rpm-build.sh.patch
index 0ed44a13..163d51d1 100644
--- a/patches/chrome-installer-linux-rpm-build.sh.patch
+++ b/patches/chrome-installer-linux-rpm-build.sh.patch
@@ -1,5 +1,5 @@
diff --git a/chrome/installer/linux/rpm/build.sh b/chrome/installer/linux/rpm/build.sh
-index 0bcd8689d45850ad539f99423fa211785db4f343..9a2d8eeb97e096db8b80aac7e26006888200e9f8 100755
+index 0bcd8689d45850ad539f99423fa211785db4f343..36e080915c22436da9f3ed8137cc6ac8338b5166 100755
--- a/chrome/installer/linux/rpm/build.sh
+++ b/chrome/installer/linux/rpm/build.sh
@@ -15,8 +15,9 @@ gen_spec() {
@@ -13,7 +13,15 @@ index 0bcd8689d45850ad539f99423fa211785db4f343..9a2d8eeb97e096db8b80aac7e2600688
local INSTALLDIR="${INSTALLDIR}-${CHANNEL}"
local PACKAGE="${PACKAGE}-${CHANNEL}"
local MENUNAME="${MENUNAME} (${CHANNEL})"
-@@ -108,7 +109,10 @@ do_package() {
+@@ -52,6 +53,7 @@ stage_install_rpm() {
+ process_template "${BUILDDIR}/installer/common/rpmrepo.cron" \
+ "${STAGEDIR}/etc/cron.daily/${PACKAGE}"
+ chmod 755 "${STAGEDIR}/etc/cron.daily/${PACKAGE}"
++ rm -rf "${STAGEDIR}/etc/cron.daily"
+ }
+
+ verify_package() {
+@@ -108,7 +110,10 @@ do_package() {
--define "${COMPRESSION_OPT}" \
--define "__os_install_post %{nil}" \
"${SPEC}"
@@ -25,7 +33,7 @@ index 0bcd8689d45850ad539f99423fa211785db4f343..9a2d8eeb97e096db8b80aac7e2600688
mv "$RPMBUILD_DIR/RPMS/$ARCHITECTURE/${PKGNAME}.${ARCHITECTURE}.rpm" \
"${OUTPUTDIR}"
# Make sure the package is world-readable, otherwise it causes problems when
-@@ -145,7 +149,10 @@ verify_channel() {
+@@ -145,7 +150,10 @@ verify_channel() {
CHANNEL=stable
;;
unstable|dev|alpha )
There was a problem hiding this comment.
Testing these packages on Fedora 29 and Ubuntu 16.04 I find that the cron.daily entries are no longer added, and the sources.list file in Ubuntu is not added. This should work as a solution to both issues: brave/brave-browser#1084 and brave/brave-browser#1967
There was a problem hiding this comment.
Looking closer, I can see that the problem is that spec file expects /etc/ to be present and so removing it without removing this copy will break the build, as you discovered.
Since there is nothing other than the undesirable cronjob that needs to be put in `/etc/ at build time, we can get rid of that directory and that copy entirely.
|
I just pushed a slightly modified set of patches to branch https://github.com/brave/brave-core/tree/PR1078 which works successfully for me to build, install and fix issues brave/brave-browser#1084 and brave/brave-browser#1967 as desired. Take a look when you get a moment @fmarier. |
|
213848e looks good to me. |
|
I will prepare an updated patch which takes #1078 (comment) into account. |
fmarier
changed the title
|
I just pushed an updated patch and while it doesn't build, the error seems unrelated: Should I build on something else than Ubuntu 18.04? |
We run builds on Ubuntu 14.04 due to packaging issues when run on newer Ubuntu versions. There is a comment explaining partially why here: #28 (comment) Chromium also builds on Ubuntu 14.04: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/linux_build_instructions.md#System-requirements For now that's our default process but we will likely want to investigate that and come up with a solution for building on newer versions going forward. |
| prep_staging_debian() { | ||
| prep_staging_common | ||
| install -m 755 -d "${STAGEDIR}/DEBIAN" \ | ||
| - "${STAGEDIR}/etc/cron.daily" \ |
There was a problem hiding this comment.
this seems like a lot of patching in different places, can't we just patch the cron script instead to do nothing?
fmarier
dismissed
bridiver’s stale review
I've changed the patch to disable the cronjobs instead of preventing their installations.
|
The build error I got in #1078 (comment) actually comes from the use of |
fmarier
force-pushed
the
issue1084
branch
2 times, most recently
from
5c576e0
to
0fba0a4
Compare
|
I have verified that this latest patch works on Ubuntu 18.04:
@mbacchi This should not be pretty straightforward to review and merge since it's now a 2-line change (excluding comments). |
|
I have also documented what I figured out in terms of building the Linux packages: https://github.com/brave/brave-browser/wiki/Linux-packages |
|
I just tested this on Fedora 29 and the Google repo is still installed at installation time, despite the cronjob being neutered. I might need to hack the packaging some more for RPM-based distros. |
This disables the cronjob that gets installed by the RPM and DEB packages in order to forcefully add (or re-add) the Google Chrome package repository and signing key. This fixes brave/brave-browser#1084 and fixes brave/brave-browser#1967.
|
Latest patch was tested successfully on Fedora 29 too. It's now 50% larger though (i.e. 3 lines of code). |
|
|
||
| -. "$DEFAULTS_FILE" | ||
| +# Don't install the Chrome repo (brave/brave-browser#1967) | ||
| +#. "$DEFAULTS_FILE" |
There was a problem hiding this comment.
I'm confused by this one, why not exit early like the others?
There was a problem hiding this comment.
That part of the spec file also sets up the brave-browser binary: https://cs.chromium.org/chromium/src/chrome/installer/linux/rpm/chrome.spec.template?l=163-180&rcl=6819376c2fbd91863cb29e02b47f653d6dc8b292
If we return early, we'll be breaking that.
There was a problem hiding this comment.
and there's nothing else we need from DEFAULTS_FILE that should be set?
There was a problem hiding this comment.
No, it only contains two variables related to adding/re-adding the Chrome repo:
$ cat /etc/default/brave-browser
repo_add_once="false"
repo_reenable_on_distupgrade="true"
|
Is there anything hindering the removal of this line? https://github.com/chromium/chromium/blob/main/chrome/installer/linux/rpm/chrome.spec.template#L88 The file would be prepared, but ultimately not included in the final RPM package. |
|
@applepine0 Good question. I don't remember whether or not we tried that at the time. It's been a while since this fix :) We are however thinking about moving away from the brave-keyring eventually (brave/brave-browser#9050) and in that case we will need the cronjob for the same reason that Chrome needs it. |
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests) ongit rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist: