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.

Sign up for GitHub

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

Jump to bottom

chromium: disable patch chromium-gn-bootstrap-r14 for version 62 #29941

Closed
wants to merge 1 commit into from
Closed

chromium: disable patch chromium-gn-bootstrap-r14 for version 62 #29941

wants to merge 1 commit into from

Conversation

disassembler
Copy link
Member

Motivation for this change

tried installing chromiumDev on 17.09 and it failed trying to apply this patch. Note that on master, this affects chromiumBeta, but on 17.09 it affects chromiumDev.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@joachifm joachifm requested a review from aszlig October 2, 2017 23:44
@aszlig
Copy link
Member

aszlig commented Oct 11, 2017

I'd like to suggest @bendlas as a reviewer instead, as I'm not maintaining Chromium anymore since a long time.

@aszlig aszlig removed their request for review October 11, 2017 03:29
bendlas
bendlas requested changes Oct 11, 2017
View reviewed changes
Copy link
Contributor

@bendlas bendlas left a comment

Choose a reason for hiding this comment

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

I fail to see how your patch should make any functional difference, except for disable the warning about outdated patches. Indeed, chromium and chromiumBeta's patch phase currently work for me. chromiumDev's (63) -r19 patch doesn't apply, but that is a different issue.

pkgs/applications/networking/browsers/chromium/common.nix
@@ -133,8 +133,10 @@ let
# https://gitweb.gentoo.org/repo/gentoo.git/plain/www-client/chromium/
# https://git.archlinux.org/svntogit/packages.git/tree/trunk?h=packages/chromium
# for updated patches and hints about build flags
++ optionals (versionRange "61" "62") [
++ optionals (versionOlder version "62") [
./patches/chromium-gn-bootstrap-r14.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm puzzling over how that should be different from the check included with versionRange: 8a5b327#diff-f286bd0559a1a8a82983af7fe0830d69L95

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is an issue with versionRange, please fix that instead of circumventing it. It also functions to warn for outdated patch sets.

@disassembler
Copy link
Member Author

You're correct, that does work now for chromiumBeta without my commit. I'm not sure why a few weeks ago it was trying to apply that patch, but the logic works fine now. I'm closing this out. My mistake was assuming versionRange was up to the upper limit on the range, but looking at the code, it just calls versionOlder, so it shouldn't have tried to apply the patch to version 62.

@disassembler
Copy link
Member Author

Ah, I see what I did wrong here... So I tested building chromiumBeta without my patch on 17.09-release (which is still broken), and then I made the changes against master, not realizing the problem was commits not cherry picked to 17.09-release. The problem with 17.09 is it doesn't do any version checking at all for that patch:

    patches = [                                                                                                                                                           
      ./patches/nix_plugin_paths_52.patch                                                                                                                                 
      ./patches/chromium-gn-bootstrap-r14.patc

The commit that fixes that is Thu Sep 21 02:32:40 2017 +0200 93aaeaccc2 chromium: separate patches for beta and dev builds [Herwig Hochleitner]

Without that patch, this happens on a build:

unpacking source archive /nix/store/wk91fg2394xg06fa39rll73546bs12iw-chromium-62.0.3202.29.tar.xz
source root is chromium-62.0.3202.29
setting SOURCE_DATE_EPOCH to timestamp 1505945226 of file chromium-62.0.3202.29/skia/ext/skia_commit_hash.h
patching sources
applying patch /nix/store/brdxf1dl5rlnxq3i7cyharnirgaq58jk-nix_plugin_paths_52.patch
patching file chrome/common/chrome_paths.cc
Hunk #1 succeeded at 76 (offset 8 lines).
Hunk #2 succeeded at 274 (offset 9 lines).
Hunk #3 succeeded at 282 (offset 9 lines).
Hunk #4 succeeded at 325 (offset 9 lines).
Hunk #5 succeeded at 360 (offset 9 lines).
applying patch /nix/store/wkc502916v4spj8rlsfmfyi507wci0mw-chromium-gn-bootstrap-r14.patch
patching file tools/gn/bootstrap/bootstrap.py
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
patch unexpectedly ends in middle of line
2 out of 2 hunks ignored -- saving rejects to file tools/gn/bootstrap/bootstrap.py.rej
builder for ‘/nix/store/56vnpc5fb0vnyvdm7y2l67jnbpnipa86-chromium-62.0.3202.29.drv’ failed with exit code 1
cannot build derivation ‘/nix/store/vc7s7ghwy09fn02inb3pc9vjkdrls34r-chromium-beta-62.0.3202.29.drv’: 1 dependencies couldn't be built
error: build of ‘/nix/store/vc7s7ghwy09fn02inb3pc9vjkdrls34r-chromium-beta-62.0.3202.29.drv’ failed

@disassembler disassembler mentioned this pull request Oct 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bendlas bendlas bendlas requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@disassembler @aszlig @bendlas