Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable distro check for linux builds to fix failing builds #149

Closed
wants to merge 1 commit into from

Conversation

bkero
Copy link
Contributor

@bkero bkero commented Jun 7, 2018

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.

Test Plan:

Run a linux build after committed. Test that the packages are installable on Trusty, Xenial, Jessie, Sid, F27.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions

@bkero bkero requested review from bbondy, RyanJarv and mbacchi June 7, 2018 18:18
@mbacchi
Copy link
Contributor

mbacchi commented Jun 7, 2018

I just remembered there's already a patch for this same file, are they applied serially and do we have other files with more than 1 patch in brave-core/patches?

@bkero
Copy link
Contributor Author

bkero commented Jun 7, 2018

This includes the previous patch contents. The pattern that I see in brave-core/patches is that it is 1 patch file per in-repo file.

@bridiver
Copy link
Collaborator

bridiver commented Jun 7, 2018

it isn't necessary to patch for this. Anything in declare_args section can be overridden in our args.gn using config.js - enable_distro_version_check

@bkero
Copy link
Contributor Author

bkero commented Jun 7, 2018

Referenced issue is brave/brave-browser#305

@bridiver
Copy link
Collaborator

bridiver commented Jun 7, 2018

some of the errors from the issue look like real problems.

01:00:29 Dependency libcairo-gobject2 (>= 1.10.0) not satisfiable on distro Ubuntu 16.04 (Xenial) caused by binary brave
01:00:29 Dependency zlib1g (>= 1:1.1.4) not satisfiable on distro Ubuntu 16.04 (Xenial) caused by binary brave

sounds like these builds may have runtime errors on those platforms. Do we know what introduced them?

@bkero
Copy link
Contributor Author

bkero commented Jun 7, 2018

I double-checked these, and packages are available for them.
For example, zlib1g on Trusty: https://packages.ubuntu.com/trusty/zlib1g
and libcairo-gobject2: https://packages.ubuntu.com/trusty/libcairo-gobject2

@bridiver
Copy link
Collaborator

bridiver commented Jun 7, 2018

they are available, but are they part of a minimal install? If not are we including them as required deps in our packages? If we are sure everything works correctly then we should probably update the list instead of turning off the check

@bkero
Copy link
Contributor Author

bkero commented Jun 7, 2018

Okay, I've adjusted the PACKAGE_FILTER in src/chrome/installer/linux/debian/update_dist_package_versions.py to include libz1g and libcairo-gobject2, regenerated the package list, and confirmed they were added.

Afterwards I ran the create_dist target again, and took a look at the resulting deb file:

$ dpkg-deb -I ../../../../out/Release/brave-amd64.deb
 new debian package, version 2.0.
 size 393769320 bytes: control archive=10408 bytes.
    1193 bytes,    13 lines      control              
   16961 bytes,   460 lines   *  postinst             #!/bin/sh
   12991 bytes,   344 lines   *  postrm               #!/bin/sh
    1364 bytes,    42 lines   *  prerm                #!/bin/sh
 Package: brave-browser
 Version: 67.0.3396.62-1
 Architecture: amd64
 Maintainer: Brave Software <support+laptop@brave.com>
 Installed-Size: 384722
 Pre-Depends: dpkg (>= 1.14.0)
 Depends: ca-certificates, fonts-liberation, libappindicator3-1, libasound2 (>= 1.0.16), libatk-bridge2.0-0 (>= 2.5.3), libatk1.0-0 (>= 1.12.4), libc6 (>= 2.14), libcairo-gobject2 (>= 1.10.0), libcairo2 (>= 1.6.0), libcups2 (>= 1.4.0), libdbus-1-3 (>= 1.2.14), libexpat1 (>= 2.0.1), libgcc1 (>= 1:3.0), libgdk-pixbuf2.0-0 (>= 2.22.0), libglib2.0-0 (>= 2.31.8), libgtk-3-0 (>= 3.9.10), libnspr4 (>= 2:4.9-2~), libnss3 (>= 2:3.22), libpango-1.0-0 (>= 1.14.0), libpangocairo-1.0-0 (>= 1.14.0), libx11-6 (>= 2:1.4.99.1), libx11-xcb1, libxcb1 (>= 1.6), libxcomposite1 (>= 1:0.3-1), libxcursor1 (>> 1.1.2), libxdamage1 (>= 1:1.1), libxext6, libxfixes3 (>= 1:5.0), libxi6 (>= 2:1.2.99.4), libxrandr2 (>= 2:1.2.99.3), libxrender1, libxss1, libxtst6, lsb-release, wget, xdg-utils (>= 1.0.2), zlib1g (>= 1:1.1.4)
 Recommends: libu2f-udev
 Provides: www-browser
 Section: web
 Priority: optional
 Description: The web browser from Brave
  Browse faster by blocking ads and trackers that violate your privacy and cost you time and money.

From this we can see that libz1g and libcairo-gobject2 are included in the dependencies. So it seems updating this would be the way forward.

@bkero
Copy link
Contributor Author

bkero commented Jun 7, 2018

Closing in favor of patching the package list generation to include these new packages.

@bkero bkero closed this Jun 7, 2018
NejcZdovc added a commit that referenced this pull request Dec 10, 2018
Trigger ac after recurring donation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants