Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRemove 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. |
|
This is the build error I get: I do seem to have |
| # Setup the installation directory hierachy in the package staging area. | ||
| prep_staging_rpm() { | ||
| prep_staging_common | ||
| - install -m 755 -d "${STAGEDIR}/etc/cron.daily" |
This comment has been minimized.
This comment has been minimized.
mbacchi
Dec 18, 2018
Member
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}" |
This comment has been minimized.
This comment has been minimized.
mbacchi
Dec 18, 2018
Member
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 )
This comment has been minimized.
This comment has been minimized.
mbacchi
Dec 19, 2018
Member
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
This comment has been minimized.
This comment has been minimized.
fmarier
Jan 16, 2019
Author
Member
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" \ |
This comment has been minimized.
This comment has been minimized.
bridiver
Jan 17, 2019
Collaborator
this seems like a lot of patching in different places, can't we just patch the cron script instead to do nothing?
|
I'm really concerned about the amount of patching in a lot of these scripts. It's going to be very difficult to maintain over time. I think we need to find a different way to do this |
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 |
|
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" |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fmarier
Jan 31, 2019
Author
Member
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.
This comment has been minimized.
This comment has been minimized.
bridiver
Jan 31, 2019
Collaborator
and there's nothing else we need from DEFAULTS_FILE that should be set?
This comment has been minimized.
This comment has been minimized.
fmarier
Feb 1, 2019
Author
Member
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"
|
These look great. Thanks! |
fmarier commentedDec 12, 2018
•
edited
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: