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

scripts to build linux installer packages #28

Merged
merged 6 commits into from
Feb 9, 2018
Merged

Conversation

AlexeyBarabash
Copy link
Contributor

This PR for issue brave/brave-browser-snap#19 .
Now command npm run create_dist or npm run create_dist -- --debug_build=true --official_build=false creates these packages:

brave-browser-chromium-fork-stable-64.0.3282.119-1.x86_64.rpm
brave-browser-chromium-fork-stable_64.0.3282.119-1_amd64.deb
brave-browser-chromium-fork-beta-64.0.3282.119-1.x86_64.rpm
brave-browser-chromium-fork-beta_64.0.3282.119-1_amd64.deb
brave-browser-chromium-fork-unstable-64.0.3282.119-1.x86_64.rpm
brave-browser-chromium-fork-unstable_64.0.3282.119-1_amd64.deb    

rpm and deb for each of channel stable, beta, unstable.

There are two points with this PR:

  1. Web repository for command line is not created yet.
  2. Build script complaints for missing dependencies during creating of rpm package. I have disabled it https://github.com/brave/antimuon/compare/linux_installer_packages?expand=1#diff-b17d27146c2b5403e627bec090114c86R10 . Tested on Fedora 27, 26 installation succeeded, probably it pulls deps from the network. If this is not appropriate, I will search for the way to include missing binaries.

@bbondy
Copy link
Member

bbondy commented Feb 1, 2018

Thanks!

  1. Web repository for command line is not created yet.

No need to work on that part, we can re-use what we do for the browser-laptop based current desktop software.

2...

I think it would be good if we can find a way to avoid that patch and have the dep.

+ "//brave/chromium_src/chrome/installer/linux/common/brave-browser/chromium-browser.appdata.xml",
+ "//brave/chromium_src/chrome/installer/linux/common/brave-browser/chromium-browser.info",
]
Copy link
Member

@bbondy bbondy Feb 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this patch can be cleaner just by removing lines 29 and 30 and replacing with lines 36 and 37

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fixed.

</p>
</description>
<url type="homepage">https://brave.com/</url>
<screenshots>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to completely remove the screenshots element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, removed.

]
packaging_files_shlibs = []

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra blank newline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding, fixed.

+ sources += [
+ "//brave/chromium_src/chrome/installer/linux/common/brave-browser/chromium-browser.appdata.xml",
+ "//brave/chromium_src/chrome/installer/linux/common/brave-browser/chromium-browser.info",
+ ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove lines 26-27 and replace with lines 36-37 here, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done.
The reason why I have done that is the message in #chromium-fork from @bridiver

when you are replacing a source file in a chrome gn file you should add a patch with `source -=` and `source +=`
vs patching inline in the existing `source =` 

But patches here is fragile balance between minimal size for future support and clarity. Direct patch does make the size of result smaller, without obvious damage for clarity. So, thanks for mention about issue.

requirement, distro, os.path.basename(binary)))
- ret_code = 1
+ # Disabled to finish the build. Some deps may be missing, but rpm is well installed.
+ #ret_code = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build of rpm packages requires to run on Ubuntu 14.
This patch is removed now.

Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments inline

if (is_chrome_branded) {
package = "google-chrome"
} else {
- package = "chromium-browser"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this name actually used? If this is a deb we don't want "-chromium-fork" here. If this package name isn't seen by users or if we can just rename it afterwards then I think we shouldn't patch it at all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line in brave/src/chrome/installer/linux/BUILD.gn does not used anywhere indeed. The package name is defined by the line PACKAGE="brave-browser" at the file https://github.com/brave/antimuon/pull/28/files#diff-a023e566679169856984037106482f04R9 . I have modified this line in prev commits to have consistency between installer/linux/BUILD.gn and chromium-browser.info. Now this is removed.

Thanks for notice, I got rid of chromium-fork mention everywhere, but I have left the package name modified, because it is taken from completely overridden file chromium_src/chrome/installer/linux/common/brave-browser/chromium-browser.info (https://github.com/brave/antimuon/pull/28/files#diff-a023e566679169856984037106482f04R1) and does not affect the patch complexity.

@AlexeyBarabash
Copy link
Contributor Author

Removed workaround patch for calculate_package_deps.py.
On Ubuntu 14 command /usr/lib/rpm/find-requires gives result without rtld(GNU_HASH).
On Ubuntu 16 even on clean install, Ubuntu 17 there is an item rtld(GNU_HASH) in output which breaks the build.
Ubuntu 14 uses rpm of version 4.11.1-3ubuntu0.1 , Ubuntu 16 uses 4.12.0.1+dfsg1-3build3, Ubuntu 17 uses 4.12.0.2+dfsg1-2build2 . And rpm version 4.11 is not available for Ubuntu 16.

Just in case the build is being configured and will be checked in Ubuntu 14.

@AlexeyBarabash
Copy link
Contributor Author

AlexeyBarabash commented Feb 6, 2018

@bbondy , @bridiver
PR is ready to have your review again. Thanks.

IOError: [Errno 2] No such file or directory: '../../chrome/app/theme/brave/BRANDING'
will add dependency in gn
don't start review

@AlexeyBarabash
Copy link
Contributor Author

Fixed issue with build after checkout. Fix is in https://github.com/brave/brave/pull/67 .
The build of all packages succeeded on Ubuntu 14.04 after checkout.

This PR is ready for review, @bbondy , @bridiver.

@bbondy
Copy link
Member

bbondy commented Feb 9, 2018

Could you make the namesbrave-x86_64.rpm and brave-amd64.deb?
If it requires a patch file in chromium src then pls just copy them to the out dir (src/out/Release)

Also secondary priority and can come in a diff PR, but pls make sure it also works with target_arch=x86

npm run build -- --target_arch=x86
npm run create_dist  -- --target_arch=x86

@bbondy bbondy merged commit 6e470cc into master Feb 9, 2018
@bsclifton bsclifton deleted the linux_installer_packages branch June 18, 2018 06:26
NejcZdovc pushed a commit that referenced this pull request Dec 10, 2018
Remove FETCH_CALLBACK_EXTRA_DATA_ST structure
bbondy added a commit that referenced this pull request Feb 18, 2019
Update @types/react-redux to fix TS2344 error during build
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