build: improve sed robustness by not using sed #19761

pull fanquake wants to merge 11 commits into bitcoin:master from fanquake:maximum_sed_robustness changing 17 files +432 −19
  1. fanquake commented at 1:39 pm on August 18, 2020: member

    While using sed can be handy to use for a quick-fix, these instances accumulate, and can become unmaintainable. Not only that, but using sed isn’t necessarily robust and it can fail silently. Most of our usage is also missing any documentation explaining why something is being done, when it should be updated/removed etc.

    Rather than relying on sed going forward, where possible, I’ve converted our sed usage into patches. These are easier to maintain, contain documentation, and should fail loudly when they don’t apply.

    The remaining sed usage, (1 in miniupnpc, the rest in qt), are non-trivial to remove, as they are using build-time variables, or some input from the environment.

    This also steals 2 related commits out of #19716.

    Related to #16838.

  2. fanquake added the label Build system on Aug 18, 2020
  3. practicalswift commented at 5:22 pm on August 18, 2020: contributor
    Concept ACK
  4. theuni commented at 6:39 pm on August 18, 2020: member

    Concept ACK. This will make simple bumps more complicated, but arguably that’s a good thing.

    Also +1 for self-documentation.

  5. laanwj commented at 9:07 am on August 19, 2020: member

    Lovely PR title :smile:

    Concept ACK. I remember having some discussions about this in the past (with @dongcarl ?). It has always bothered me that sed can fail silently. This is especially problematic because changes can be lost without noticing when bumping versions. This has in the past resulted in losing propagation of hardening flags and such.

  6. hebasto commented at 10:23 am on August 19, 2020: member
    Concept ACK.
  7. dongcarl commented at 3:12 pm on August 19, 2020: member

    Concept ACK! All the ways I’ve tried fixing this before weren’t satisfactory, and this is a nice and simple approach.

    One q: I often see -N being using in most patch invocations, would we want that?

  8. fanquake force-pushed on Aug 20, 2020
  9. fanquake commented at 1:27 pm on August 20, 2020: member

    I’m glad everyone agrees with the approach. I’ve pushed a fix the docs in one patch file.

    One q: I often see -N being using in most patch invocations, would we want that?

    I’ve done some testing and think it could be a worthwhile addition. It seems if anything it causes patch to “fail faster”. i.e chain applying the same patch with and without it:

     0# no -N
     1patch -p1 -i a.patch && patch -p1 -i a.patch && patch -p1 -i a.patch
     2patching file test.cpp
     3patching file test.cpp
     4Reversed (or previously applied) patch detected!  Assume -R? [n] n
     5Apply anyway? [n] n
     6Skipping patch.
     71 out of 1 hunk ignored -- saving rejects to file test.cpp.rej
     8
     9# reset
    10git stash
    11Saved working directory and index state WIP on master: cdb5127 init
    12
    13# Using -N
    14patch -N -p1 -i a.patch && patch -N -p1 -i a.patch && patch -N -p1 -i a.patch
    15patching file test.cpp
    16patching file test.cpp
    17Reversed (or previously applied) patch detected!  Skipping patch.
    181 out of 1 hunk ignored -- saving rejects to file test.cpp.rej
    

    So without -N you’ll be asked for input, as opposed to just failing.

  10. DrahtBot commented at 7:56 pm on August 20, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19785 (build: lrelease requires xml if not cross-building by hebasto)
    • #19764 (depends: Split boost into build/host packages + bump + cleanup by dongcarl)
    • #19716 (build: Qt 5.15.x by fanquake)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  11. in depends/packages/qt.mk:196 in 5b121cb9e8 outdated
    192@@ -193,7 +193,6 @@ define $(package)_preprocess_cmds
    193   sed -i.old "s|FT_Get_Font_Format|FT_Get_X11_Font_Format|" qtbase/src/platformsupport/fontdatabases/freetype/qfontengine_ft.cpp && \
    194   sed -i.old "s|updateqm.commands = \$$$$\$$$$LRELEASE|updateqm.commands = $($(package)_extract_dir)/qttools/bin/lrelease|" qttranslations/translations/translations.pro && \
    195   sed -i.old "/updateqm.depends =/d" qttranslations/translations/translations.pro && \
    196-  sed -i.old "s/src_plugins.depends = src_sql src_network/src_plugins.depends = src_network/" qtbase/src/src.pro && \
    


    hebasto commented at 11:47 am on August 22, 2020:
    Happy with that! Was going to submit a pull to remove it :)

    theuni commented at 8:07 pm on August 26, 2020:

    Could you please explain the removal? I’m sure it’s fine, I’m just curious what’s changed upstream.

    IIRC this was purely a build speedup, just skipping building sql stuff. I suppose qt now correctly avoids building that when not needed?


    hebasto commented at 9:17 pm on August 26, 2020:

    https://code.qt.io/cgit/qt/qtbase.git/tree/src/src.pro?h=5.9.8#n153:

    0qtConfig(network) {
    1    SUBDIRS += src_network
    2    src_plugins.depends += src_network
    3}
    4qtConfig(sql) {
    5    SUBDIRS += src_sql
    6    src_plugins.depends += src_sql
    7}
    
  12. hebasto commented at 9:11 am on August 24, 2020: member

    Reviewing commits:

    • 5f98794770b7152280ab388cf1682a73f772e99a “build: use patch rather than sed in bdb package”
    • 53afb710a3a05f813b46a5959e7d9047080e54d4 “build: use patch rather than sed in Boost package”
    • 45f33606377cda7065fe30aa4eeee5033e9b0699 “build: use patch rather than sed in fontconfig package”
    • b2140672ee708441e1652b8153400b852c522f1c “build: use patch rather than sed in native_cctools package”
    • e995635d3c3f8e87c982411e3eb59e135b16b00b “build: use patch rather than sed in zeromq package”
    • 6d4243925c63d7d94c7360a5d469f6e6533e71a3 “build: remove no-longer needed qt configure workaround”
    • 5b121cb9e88e134fe8e6a510e5a7386cc9d6060d “build: remove no-longer needed qt workaround”
    • 5bdea1d09a5f5c59f5daf97881202e0fd7db2ef5 “build: replace pwd sed in qt package with a patch”
    • e2b18a4120f344d2043329f0382e77e7fe9ae933 “build: replace FreeType back-compat sed with a patch in qt package”
    • 37d989859e275adac80c9ccf218f5024336f0de2 “build: replace qtranslations ltranslate sed with a patch in qt package”
    • 0b0f480765a7aba50371e11a6047cebd322ac12c “build: replace wingenminiupnpcstrings sed with a patch in miniupnpc package”
  13. in depends/patches/fontconfig/gperf_header_regen.patch:15 in 45f3360637 outdated
    10+    See #10851.
    11+
    12+diff --git a/src/Makefile b/src/Makefile
    13+index aa9ea57..ded34a8 100644
    14+--- a/src/Makefile
    15++++ b/src/Makefile
    


    hebasto commented at 10:42 am on August 24, 2020:

    45f33606377cda7065fe30aa4eeee5033e9b0699

    I suggest to apply this patch to Makefile.in and move all patching work into the preprocess_cmds stage. Patching the source file rather the generated one is simpler to verify and maintain, no?


    fanquake commented at 6:51 am on August 25, 2020:
    I’ve moved the patching to preprocessing.
  14. in depends/patches/qt/freetype_back_compat.patch:14 in e2b18a4120 outdated
     9+    backwards-compatibility.
    10+
    11+    Qt 5.9 introduced a call to FT_Get_Font_Format(). Replace it with FT_Get_X11_Font_Format()
    12+    in order to remain compatibile with older freetype, which is still used by e.g. Ubuntu Trusty.
    13+
    14+    See 14348.
    


    hebasto commented at 3:29 pm on August 24, 2020:

    e2b18a4120f344d2043329f0382e77e7fe9ae933 nano-nit:

    0    See [#14348](/bitcoin-bitcoin/14348/).
    

    fanquake commented at 5:50 am on August 25, 2020:
    Will be fixed in the next push.
  15. in depends/patches/qt/drop_ltranslate_dependency.patch:7 in 37d989859e outdated
    0@@ -0,0 +1,24 @@
    1+commit 67b3ed7406e1d0762188dbad2c44a06824ba0778
    2+Author: fanquake <fanquake@gmail.com>
    3+Date:   Tue Aug 18 15:24:01 2020 +0800
    4+
    5+    Drop dependency on ltranslate
    6+
    7+    Qts buildsystem insists on using the installed ltranslate, but gets
    


    hebasto commented at 4:23 pm on August 24, 2020:

    37d989859e275adac80c9ccf218f5024336f0de2 Here

    0    Qts buildsystem insists on using the installed lrelease, but gets
    

    and in the patch name.


    fanquake commented at 5:50 am on August 25, 2020:
    Updated in the next push.
  16. in depends/patches/miniupnpc/dont_use_wingen.patch:9 in 0b0f480765 outdated
    0@@ -0,0 +1,23 @@
    1+commit e8077044df239bcf0d9e9980b0e1afb9f1f5c446
    2+Author: fanquake <fanquake@gmail.com>
    3+Date:   Tue Aug 18 20:50:19 2020 +0800
    4+
    5+    Don't use wingenminiupnpcstrings when generating miniupnpcstrings.h
    6+
    7+    The wingenminiupnpcstrings tool is used on Windows to generate version
    8+    information. This information is irrelevant for us, and trying to use
    9+    wingenminiupnpcstrings would cause builds to fail, so just don't use it.
    


    hebasto commented at 4:55 pm on August 24, 2020:
    Is it worth to mention that this patch could be skipped on miniupnpc versions 2.1+ ? See: https://github.com/miniupnp/miniupnp/commit/9663c55c61408fdcc39a82987d2243f816b22932

    fanquake commented at 5:55 am on August 25, 2020:
    Will be added in the next push.
  17. in depends/packages/miniupnpc.mk:17 in 0b0f480765 outdated
    14@@ -14,7 +15,7 @@ endef
    15 define $(package)_preprocess_cmds
    16   mkdir dll && \
    17   sed -e 's|MINIUPNPC_VERSION_STRING \"version\"|MINIUPNPC_VERSION_STRING \"$($(package)_version)\"|' -e 's|OS/version|$(host)|' miniupnpcstrings.h.in > miniupnpcstrings.h && \
    18-  sed -i.old "s|miniupnpcstrings.h: miniupnpcstrings.h.in wingenminiupnpcstrings|miniupnpcstrings.h: miniupnpcstrings.h.in|" Makefile.mingw
    


    hebasto commented at 5:10 pm on August 24, 2020:
    Oh, apparently, both sed commands fix one issue with cross-compiling for Windows. Maybe it would better do not touch them at all (as we are not able to transform both to patch), no?

    fanquake commented at 5:56 am on August 25, 2020:
    I think it’s fine. I also think we can probably drop (at least the version part of) the other sed. From what I can tell the output will be deterministic.

    hebasto commented at 5:58 am on August 25, 2020:

    I think it’s fine. I also think we can probably drop (at least the version part of) the other sed. From what I can tell the output will be deterministic.

    I’ve tried. Windows cross-compiling fails.


    hebasto commented at 6:00 am on August 25, 2020:
    We could drop it if patch Makefile.mingw to do not use wingenminiupnpcstrings for the miniupnpcstrings.h target.

    fanquake commented at 6:02 am on August 25, 2020:

    I’ve tried. Windows cross-compiling fails.

    I think we are talking about different things. I’m talking about dropping -e 's|MINIUPNPC_VERSION_STRING \"version\"|MINIUPNPC_VERSION_STRING \"$($(package)_version)\"|' from the sed usage above. I’m not sure why that would cause a compile failure.


    hebasto commented at 6:05 am on August 25, 2020:
    It will generate ill-formed miniupnpcstrings.h, no?

    hebasto commented at 6:09 am on August 25, 2020:

    https://github.com/miniupnp/miniupnp/blob/1337158fcff401def996e211d5940c9f74f92b51/miniupnpc/Makefile.mingw#L77-L78:

    0miniupnpcstrings.h: miniupnpcstrings.h.in wingenminiupnpcstrings
    1	wingenminiupnpcstrings $< $@
    

    fanquake commented at 6:18 am on August 25, 2020:
    Everything compiles, you just end up with an incorrect User-Agent: User-Agent: x86_64-w64-mingw32, UPnP/1.1, MiniUPnPc/version. I’ll leave this as and we can follow up if/when miniupnpc is next updated. I’ve updated the patch to mention upstream changes.
  18. hebasto changes_requested
  19. hebasto commented at 5:17 pm on August 24, 2020: member

    Approach ACK 0b0f480765a7aba50371e11a6047cebd322ac12c.

    I’ve verified that patch changes are equal to sed changes. I’ve verified that dropped workarounds are effectively noop.

  20. DrahtBot commented at 3:41 am on August 25, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  21. DrahtBot added the label Needs rebase on Aug 25, 2020
  22. build: use patch rather than sed in bdb package f36140d00c
  23. build: use patch rather than sed in Boost package
    The depends comment is actually incorrect, and this can be dropped once
    we move to 1.71.0 or later.
    335bd7f8bc
  24. build: use patch rather than sed in fontconfig package 865cb23a48
  25. build: use patch rather than sed in native_cctools package cc107a3af1
  26. build: use patch rather than sed in zeromq package 4af59a407a
  27. build: remove no-longer needed qt configure workaround
    This was fixed upstream in c45595d64831990311f92fcebc4e34e2797f5352.
    bf85eace1a
  28. build: remove no-longer needed qt workaround 9d440f4e11
  29. build: replace pwd sed in qt package with a patch 3aaa39d436
  30. build: replace FreeType back-compat sed with a patch in qt package c723e4176e
  31. fanquake force-pushed on Aug 25, 2020
  32. fanquake removed the label Needs rebase on Aug 25, 2020
  33. fanquake added the label Needs gitian build on Aug 25, 2020
  34. fanquake added the label Needs Guix build on Aug 25, 2020
  35. hebasto approved
  36. hebasto commented at 7:18 am on August 25, 2020: member

    ACK 83445516f20ffe2e170237e13e66da64852c7a14, only rebased and suggested changes applied since my previous review.

    nit: in 83445516f20ffe2e170237e13e66da64852c7a14 would it better to place a commit message into the patch code?

    btw, what commits are referred to in patch fist lines? GitHub fails to find them :)

    Running gitian builds now…

  37. fanquake force-pushed on Aug 25, 2020
  38. fanquake commented at 7:28 am on August 25, 2020: member

    nit: in 8344551 would it better to place a commit message into the patch code?

    Thanks. This is what I meant to do, but I only added it to the commit body.

    btw, what commits are referred to in patch fist lines? GitHub fails to find them :)

    I’ve just been creating temporary local repos using the package source; so none of the commits have been pushed anywhere.

  39. in depends/patches/miniupnpc/dont_use_wingen.patch:24 in 03354f8479 outdated
    19+
    20+ wingenminiupnpcstrings.o:	wingenminiupnpcstrings.c
    21+
    22+-miniupnpcstrings.h: miniupnpcstrings.h.in wingenminiupnpcstrings
    23++miniupnpcstrings.h: miniupnpcstrings.h.in
    24+ 	wingenminiupnpcstrings $< $@
    


    hebasto commented at 7:59 am on August 25, 2020:
    Probably, removing miniupnpcstrings.h target completely is more correct patch.

    fanquake commented at 8:03 am on August 25, 2020:
    I’d rather keep these as the equivalents of the commands they are replacing.

    hebasto commented at 8:06 am on August 25, 2020:
     0--- Makefile.mingw.OLD	2016-01-19 12:26:58.000000000 +0200
     1+++ Makefile.mingw.NEW	2020-08-25 11:03:57.144468741 +0300
     2@@ -70,13 +70,6 @@
     3 listdevices: listdevices.o libminiupnpc.a
     4 	$(CC) -o $@ $^ $(LDLIBS)
     5 
     6-wingenminiupnpcstrings:	wingenminiupnpcstrings.o
     7-
     8-wingenminiupnpcstrings.o:	wingenminiupnpcstrings.c
     9-
    10-miniupnpcstrings.h: miniupnpcstrings.h.in wingenminiupnpcstrings
    11-	wingenminiupnpcstrings $< $@
    12-
    13 minixml.o:	minixml.c minixml.h
    14 
    15 upnpc.o:	miniwget.h minisoap.h miniupnpc.h igd_desc_parse.h
    

    hebasto commented at 8:06 am on August 25, 2020:

    I’d rather keep these as the equivalents of the commands they are replacing.

    ok


    theuni commented at 8:14 pm on August 26, 2020:
    Agree with both of these comments :)
  40. hebasto approved
  41. hebasto commented at 9:34 am on August 25, 2020: member

    re-ACK 03354f8479fc5af9975956b8e3d8bc3c821ec268

    My gitian builds:

     0862d587b521881fcf6477e1ccead07569c7feb34a1522d8b10cd4bf28f504f42  bitcoin-03354f8479fc-aarch64-linux-gnu-debug.tar.gz
     1719ccbed4f611ee59b838125cc9c02a5896c61d1036643d31de2cb518c546e69  bitcoin-03354f8479fc-aarch64-linux-gnu.tar.gz
     260d0a7457c33ae17989c57151285811cf91dceea5ebe88be3621e0d0ad6a6303  bitcoin-03354f8479fc-arm-linux-gnueabihf-debug.tar.gz
     3ccf28e2cb699a281906c9823a667561c2f7bfe2f7cd1fd95f44fbb844fcb72dd  bitcoin-03354f8479fc-arm-linux-gnueabihf.tar.gz
     435232596d3942c4e2e704127c93a60d6efe15f8378a848e7be0e1aac69a27083  bitcoin-03354f8479fc-riscv64-linux-gnu-debug.tar.gz
     5f5e1128d67c8a03aa4a6515a22c9676089b9a771408c80c71caed1570bcfa8df  bitcoin-03354f8479fc-riscv64-linux-gnu.tar.gz
     6252a7726d396224ca3d57d5dd97bf05ddf11331912bc967d976136e81c11158f  bitcoin-03354f8479fc-x86_64-linux-gnu-debug.tar.gz
     7cbee1ae54e10531dd0dbc8d5eef013dd60b67ec333877f0d91a318e6e5ed7a8c  bitcoin-03354f8479fc-x86_64-linux-gnu.tar.g
     8cd3f7b2643d8379b2fe9fa2916681615001415c16aaa27ae2f285908ec05863d  bitcoin-03354f8479fc-win-unsigned.tar.gz
     9e9a165adb7083320857f32fc9d7b215ef76959d239a4271e822855acddd524f5  bitcoin-03354f8479fc-win64-debug.zip
    109c2e5a9cab9449ba6fa54892782d9859733ac5b3997ba59510f1a3aaf5b6b9b0  bitcoin-03354f8479fc-win64-setup-unsigned.exe
    117d91f0326d1cb931160b9c6e67cc284e8b3c827197ae3cd785e3a3a1d2877321  bitcoin-03354f8479fc-win64.zip
    123f02922e1847bcef5828ffd8b3624ad8a3144ec7d61569d06f780f02f8cd82eb  bitcoin-03354f8479fc-osx-unsigned.dmg
    1398bdd192dd0e0e0f26903121f124b1cce846dc54b74701b8fc1df68017aa5fe5  bitcoin-03354f8479fc-osx-unsigned.tar.gz
    1473cb582aa0232a72f66a94758fccc10a32eed3da56b99bde9082d49b0e5131ae  bitcoin-03354f8479fc-osx64.tar.gz
    1529235bdaf82418d0609d578cd35813e139bf2246177fa956aa7db0371f86b7a9  src/bitcoin-03354f8479fc.tar.gz
    

    patch logs

    0patching file qttranslations/translations/translations.pro
    1Hunk [#1](/bitcoin-bitcoin/1/) succeeded at 107 with fuzz 2.
    

    are not a problem.


    UPDATE: Actually, they are a problem. See: #19761 (comment)

  42. MarcoFalke commented at 1:42 pm on August 25, 2020: member
    I think travis fails in the line in the yaml that says Temporary workaround for [#16368](/bitcoin-bitcoin/16368/). Maybe this needs the ..4 changed to ..5 as another workaround? (Unless you want to fix the issue)
  43. hebasto commented at 3:59 pm on August 25, 2020: member

    Hmmm… Travis fails on ARM due to the patch error (https://travis-ci.org/github/hebasto/bitcoin/builds/721048554#L2040):

    0patching file qttranslations/translations/translations.pro
    1Hunk 1 FAILED 107/107.
    2 updateqm.output = $$MODULE_BASE_OUTDIR/translations/${QMAKE_FILE_BASE}.qm
    3 updateqm.commands = $$LRELEASE ${QMAKE_FILE_IN} -qm ${QMAKE_FILE_OUT}
    4 silent:updateqm.commands = [@echo](/bitcoin-bitcoin/contributor/echo/) lrelease ${QMAKE_FILE_IN} && $$updateqm.commands
    5-updateqm.depends = $$LRELEASE_EXE
    6 updateqm.name = LRELEASE ${QMAKE_FILE_IN}
    7 updateqm.CONFIG += no_link no_clean target_predeps
    8 QMAKE_EXTRA_COMPILERS += updateqm
    
  44. MarcoFalke commented at 4:28 pm on August 25, 2020: member
    oh, maybe it needs the fuzz resolved cleanly in the patch file?
  45. hebasto commented at 4:29 pm on August 25, 2020: member

    oh, maybe it needs the fuzz resolved cleanly in the patch file?

    testing right now :)

  46. hebasto commented at 4:32 pm on August 25, 2020: member

    oh, maybe it needs the fuzz resolved cleanly in the patch file?

    Travis ARM build uses BusyBox’s patch command.

  47. in depends/patches/qt/drop_lrelease_dependency.patch:19 in 2c5c413449 outdated
    16++++ b/qttranslations/translations/translations.pro
    17+@@ -107,7 +107,6 @@ updateqm.input = TRANSLATIONS
    18+ updateqm.output = $$MODULE_BASE_OUTDIR/translations/${QMAKE_FILE_BASE}.qm
    19+ updateqm.commands = $$LRELEASE ${QMAKE_FILE_IN} -qm ${QMAKE_FILE_OUT}
    20+ silent:updateqm.commands = @echo lrelease ${QMAKE_FILE_IN} && $$updateqm.commands
    21+-updateqm.depends = $$LRELEASE_EXE
    


    hebasto commented at 5:16 pm on August 25, 2020:

    2c5c41344976cf4aba45621a8b99e6df3a2de33b

    The following change makes busybox patch command works correctly, and it fixes Travis ARM build:

    0-updateqm.depends = $$LRELEASE_EXE
    1+
    

    hebasto commented at 5:32 pm on August 25, 2020:
    Hmm, it seems work for busybox patch only.
  48. MarcoFalke commented at 6:10 pm on August 25, 2020: member

    I think busybox can’t handle fuzz. Does removing it help?

     0diff --git a/depends/patches/qt/drop_lrelease_dependency.patch b/depends/patches/qt/drop_lrelease_dependency.patch
     1index 8b3472b675..75aa5f2629 100644
     2--- a/depends/patches/qt/drop_lrelease_dependency.patch
     3+++ b/depends/patches/qt/drop_lrelease_dependency.patch
     4@@ -14,11 +14,7 @@ diff --git a/qttranslations/translations/translations.pro b/qttranslations/trans
     5 index 694544c..eff339d 100644
     6 --- a/qttranslations/translations/translations.pro
     7 +++ b/qttranslations/translations/translations.pro
     8-@@ -107,7 +107,6 @@ updateqm.input = TRANSLATIONS
     9- updateqm.output = $$MODULE_BASE_OUTDIR/translations/${QMAKE_FILE_BASE}.qm
    10- updateqm.commands = $$LRELEASE ${QMAKE_FILE_IN} -qm ${QMAKE_FILE_OUT}
    11+@@ -109,3 +109,2 @@ updateqm.commands = /home/marco/workspace/btc_bitcoin_core/depends/work/build/x8
    12  silent:updateqm.commands = [@echo](/bitcoin-bitcoin/contributor/echo/) lrelease ${QMAKE_FILE_IN} && $$updateqm.commands
    13 -updateqm.depends = $$LRELEASE_EXE
    14  updateqm.name = LRELEASE ${QMAKE_FILE_IN}
    15- updateqm.CONFIG += no_link no_clean target_predeps
    16- QMAKE_EXTRA_COMPILERS += updateqm
    
  49. hebasto commented at 6:13 pm on August 25, 2020: member

    ~Here is a branch with two fix commits on top: https://github.com/hebasto/bitcoin/commits/pr19761-fixed2~

    UPDATE: Marco’s solution works.

  50. hebasto commented at 11:20 pm on August 25, 2020: member

    I think busybox can’t handle fuzz. Does removing it help?

     0diff --git a/depends/patches/qt/drop_lrelease_dependency.patch b/depends/patches/qt/drop_lrelease_dependency.patch
     1index 8b3472b675..75aa5f2629 100644
     2--- a/depends/patches/qt/drop_lrelease_dependency.patch
     3+++ b/depends/patches/qt/drop_lrelease_dependency.patch
     4@@ -14,11 +14,7 @@ diff --git a/qttranslations/translations/translations.pro b/qttranslations/trans
     5 index 694544c..eff339d 100644
     6 --- a/qttranslations/translations/translations.pro
     7 +++ b/qttranslations/translations/translations.pro
     8-@@ -107,7 +107,6 @@ updateqm.input = TRANSLATIONS
     9- updateqm.output = $$MODULE_BASE_OUTDIR/translations/${QMAKE_FILE_BASE}.qm
    10- updateqm.commands = $$LRELEASE ${QMAKE_FILE_IN} -qm ${QMAKE_FILE_OUT}
    11+@@ -109,3 +109,2 @@ updateqm.commands = /home/marco/workspace/btc_bitcoin_core/depends/work/build/x8
    12  silent:updateqm.commands = [@echo](/bitcoin-bitcoin/contributor/echo/) lrelease ${QMAKE_FILE_IN} && $$updateqm.commands
    13 -updateqm.depends = $$LRELEASE_EXE
    14  updateqm.name = LRELEASE ${QMAKE_FILE_IN}
    15- updateqm.CONFIG += no_link no_clean target_predeps
    16- QMAKE_EXTRA_COMPILERS += updateqm
    

    It works: https://travis-ci.org/github/hebasto/bitcoin/builds/721158783

  51. hebasto commented at 11:24 pm on August 25, 2020: member
    @fanquake If/when you will update this pull, mind removing intermediate commit ids as they seem confusing.
  52. hebasto commented at 0:00 am on August 26, 2020: member
    I’ve made gitian builds with Marco’s suggestion, and there are no “Hunk succeeded … with fuzz” in logs.
  53. build: replace qtranslations lrelease sed with a patch in qt package bbc01a753d
  54. build: replace wingenminiupnpcstrings sed with a patch in miniupnpc package
    We should be able to drop this once we are using 2.1 or later. See
    upstream commit: 9663c55c61408fdcc39a82987d2243f816b22932.
    3de365e4f1
  55. fanquake force-pushed on Aug 26, 2020
  56. fanquake commented at 5:15 am on August 26, 2020: member

    I’ve updated bbc01a753d40488fa1469c87b31e7a12ddc8b329 to fix the patch. Is there a distro that currently uses busybox patch out of the box? Even Alpine, which is based on busybox, now uses GNU patch by default.

    0patch --version
    1GNU patch 2.7.6
    2Copyright (C) 2003, 2009-2012 Free Software Foundation, Inc.
    3Copyright (C) 1988 Larry Wall
    

    I had sanity checked on 3.12 before opening, but given it’s now using GNU patch I didn’t see any issues.

    If you try building the previous version of this branch on Alpine 3.11, which is still using busybox patch:

    0patch --help
    1BusyBox v1.31.1 () multi-call binary.
    2
    3Usage: patch [OPTIONS] [ORIGFILE [PATCHFILE]]
    4
    5	-p N	Strip N leading components from file names
    6....
    

    then you’ll see the failure:

     0/bitcoin/depends/sources/qttools-opensource-src-5.9.8.tar.xz: OK
     1Preprocessing qt...
     2patching file qtbase/src/platformsupport/fontdatabases/freetype/qfontengine_ft.cpp
     3patching file qttranslations/translations/translations.pro
     4Hunk 1 FAILED 107/107.
     5 updateqm.output = $$MODULE_BASE_OUTDIR/translations/${QMAKE_FILE_BASE}.qm
     6 updateqm.commands = $$LRELEASE ${QMAKE_FILE_IN} -qm ${QMAKE_FILE_OUT}
     7 silent:updateqm.commands = [@echo](/bitcoin-bitcoin/contributor/echo/) lrelease ${QMAKE_FILE_IN} && $$updateqm.commands
     8-updateqm.depends = $$LRELEASE_EXE
     9 updateqm.name = LRELEASE ${QMAKE_FILE_IN}
    10 updateqm.CONFIG += no_link no_clean target_predeps
    11 QMAKE_EXTRA_COMPILERS += updateqm
    12make: *** [funcs.mk:263: /bitcoin/depends/work/build/x86_64-pc-linux-musl/qt/5.9.8-da7fc57da02/.stamp_preprocessed] Error 1
    13make: Leaving directory '/bitcoin/depends'
    
  57. MarcoFalke commented at 5:40 am on August 26, 2020: member
    ok, then maybe commit 8f8af457d282534bbe3678eb068ae671f7f74da2 (skip busybox patch) is the way to go
  58. fanquake commented at 5:44 am on August 26, 2020: member

    ok, then maybe commit 8f8af45 (skip busybox patch) is the way to go

    I’d rather leave this as is for now. People will still be be building on older versions of Alpine (it’s also used as a base for a lot of Docker containers), so we might as well have a sanity check that our patches work.

  59. hebasto approved
  60. hebasto commented at 8:06 am on August 26, 2020: member
    re-ACK 3de365e4f15ddaaf6bdb5b3c31529156617d2835, only drop_lrelease_dependency.patch updated. Travis makes ARM build without errors now.
  61. DrahtBot commented at 3:38 pm on August 26, 2020: member

    Guix builds

    File commit f8462a6d2794be728cf8550f45d19a354aae59cf(master) commit d8cd7fb58afe02e6d9521da1b74876bc78d3987e(master and this pull)
    *-aarch64-linux-gnu-debug.tar.gz 8ec75599e3120659... 8a8363d3e6247f20...
    *-aarch64-linux-gnu.tar.gz 751497d0edfcc7bd... 5709fd88b46f0e53...
    *-arm-linux-gnueabihf-debug.tar.gz 691dbcc01977d331... bfa4900da6dd0be3...
    *-arm-linux-gnueabihf.tar.gz 12d84add93f32cd9... 64e352e9e31d591a...
    *-riscv64-linux-gnu-debug.tar.gz 67a668b6b3ec26a6... 1abd84ce650d03fa...
    *-riscv64-linux-gnu.tar.gz 0d617846da4c989a... 5a07e56e61664df4...
    *-win-unsigned.tar.gz 6c78303889b183bb... 8d4b45cda0339531...
    *-win64-debug.zip 29c3e57e847d36c3... ae8843c5191ad440...
    *-win64-setup-unsigned.exe 254c9d2ab0a14cdf... 02a04aa103c2087f...
    *-win64.zip 3f22f689045a1323... 37420b0a18fdef11...
    *-x86_64-linux-gnu-debug.tar.gz 342496412b68d38e... afbda3ab97883393...
    *-x86_64-linux-gnu.tar.gz 04b09e1dbdd0803f... 7d72ddec94a74b79...
    *.tar.gz 17f7daeddb49ed69... ee68dc471ad6e058...
    guix_build.log d9d1fd2f925b67ac... e1a86e4010938c15...
    guix_build.log.diff 756b62bdafa9f99c...
  62. DrahtBot removed the label Needs Guix build on Aug 26, 2020
  63. in depends/patches/qt/drop_lrelease_dependency.patch:9 in bbc01a753d outdated
    0@@ -0,0 +1,20 @@
    1+commit 67b3ed7406e1d0762188dbad2c44a06824ba0778
    2+Author: fanquake <fanquake@gmail.com>
    3+Date:   Tue Aug 18 15:24:01 2020 +0800
    4+
    5+    Drop dependency on lrelease
    6+
    7+    Qts buildsystem insists on using the installed lrelease, but gets
    8+    confused about how to find it. Since we manually control the build
    9+    order, just drop the dependency.
    


    theuni commented at 8:33 pm on August 26, 2020:

    I believe this is/was only necessary because of the funky way we build qt to avoid building a bunch of unneeded stuff. If we ever move to a simple ‘make’ for qt (rather than the 4 we do now), it should be able to handle all of this for us.

    That may be worth investigating as a follow-up, it’s been a long time since we’ve revisited it. Maybe qt’s build has reduced unnecessary compiles since then.


    fanquake commented at 6:38 am on August 27, 2020:
    A revisit into how we are building Qt will happen as part of #19716 (which is also why I wanted to keep this a sed -> patch migration with no additional changes). I think there’s a good chance a bunch of complexity is going to disappear.
  64. theuni approved
  65. theuni commented at 8:34 pm on August 26, 2020: member

    Loving this. No opinion on the busybox/fuzz stuff.

    I didn’t actually verify by hand that the patched result matches the sed result, but the patches and descriptions look good to me.

    ACK 3de365e4f15ddaaf6bdb5b3c31529156617d2835.

  66. fanquake merged this on Aug 27, 2020
  67. fanquake closed this on Aug 27, 2020

  68. DrahtBot commented at 4:09 pm on August 27, 2020: member

    Gitian builds

    File commit 93ab136a33e46080c8aa02d59fb7c2a8d03a3387(master) commit cc6c98901ce0a8e9e1af6181a3c5713048dc1793(master and this pull)
    *-aarch64-linux-gnu-debug.tar.gz d9677652e379d0fb... 7c67dea5c5ef1948...
    *-aarch64-linux-gnu.tar.gz ccea6adc4185f412... dbae531b76d2c67e...
    *-arm-linux-gnueabihf-debug.tar.gz a6eb099566fbfaf7... 649441b0c69db243...
    *-arm-linux-gnueabihf.tar.gz a778747de3c362d3... fa027b55bf33a0d1...
    *-osx-unsigned.dmg 7f2fe092602c5f24... fdf3861ec6226680...
    *-osx64.tar.gz 572ad9470570b795... 0aacf000941ff808...
    *-riscv64-linux-gnu-debug.tar.gz e3c3530a641bf3ff... 1921f9ed6ec6365a...
    *-riscv64-linux-gnu.tar.gz 9e35a022bcbb24b5... 89bc6abe015e1240...
    *-win64-debug.zip 183ec1685b274201... efc0a2731dece71b...
    *-win64-setup-unsigned.exe b0345c819e39cf5b... fe3c02ae38d0495b...
    *-win64.zip ee5279efd7b75303... 854aa51f4bbe42b3...
    *-x86_64-linux-gnu-debug.tar.gz 1528d365bd4947b0... 108aa7e95ce9a02e...
    *-x86_64-linux-gnu.tar.gz 721e213b9f0ada44... 45b1e9f0ca14c1ed...
    *.tar.gz 5e4e2d09bfcdc38c... 4c90561d7cc23a1d...
    bitcoin-core-linux-0.21-res.yml 0366af80fb44ac95... 2c567235eb9698a9...
    bitcoin-core-osx-0.21-res.yml 1635b70517fa98f9... 9711d8ea68dc5b87...
    bitcoin-core-win-0.21-res.yml 25db96826213443f... 5edfc9013f7e9f2c...
    linux-build.log 6c3144ff05037c6e... 79670ba35b42a5fa...
    osx-build.log ce3a375d3051d5ba... 56d3f3ea5475a574...
    win-build.log 3a0f70b25edb7239... 246ed7b3a3d11961...
    bitcoin-core-linux-0.21-res.yml.diff 536223d15702e735...
    bitcoin-core-osx-0.21-res.yml.diff 40e4abf1b033b4be...
    bitcoin-core-win-0.21-res.yml.diff 812726c22d9587a0...
    linux-build.log.diff 8da92c0824a32be5...
    osx-build.log.diff 30f2af58a2471eac...
    win-build.log.diff 5e4f34194c79204b...
  69. DrahtBot removed the label Needs gitian build on Aug 27, 2020
  70. sidhujag referenced this in commit a012bea252 on Aug 28, 2020
  71. fanquake deleted the branch on Sep 22, 2020
  72. jasonbcox referenced this in commit 67f690d306 on Oct 5, 2020
  73. zkbot referenced this in commit a2f85199f0 on Oct 12, 2020
  74. fanquake referenced this in commit e9a1c9fbde on Nov 22, 2020
  75. ftrader referenced this in commit 354e500437 on Aug 13, 2021
  76. kittywhiskers referenced this in commit bcb6728aa0 on Aug 24, 2021
  77. kittywhiskers referenced this in commit 0938f1284b on Aug 24, 2021
  78. kittywhiskers referenced this in commit 501393f6a0 on Aug 24, 2021
  79. kittywhiskers referenced this in commit 5751c63791 on Aug 24, 2021
  80. kittywhiskers referenced this in commit e358c681c2 on Aug 24, 2021
  81. kittywhiskers referenced this in commit 953e7f5b32 on Aug 24, 2021
  82. kittywhiskers referenced this in commit f6989e87cb on Aug 24, 2021
  83. kittywhiskers referenced this in commit 597b4bb3cc on Aug 25, 2021
  84. kittywhiskers referenced this in commit 274fc47c57 on Aug 25, 2021
  85. kittywhiskers referenced this in commit 8351d806a0 on Aug 26, 2021
  86. kittywhiskers referenced this in commit fb937ca8a3 on Aug 26, 2021
  87. kittywhiskers referenced this in commit 5dc2a1b3f2 on Aug 26, 2021
  88. kittywhiskers referenced this in commit 409bf1963f on Aug 27, 2021
  89. kittywhiskers referenced this in commit 41bf4916a6 on Aug 30, 2021
  90. kittywhiskers referenced this in commit a4377af5ef on Sep 1, 2021
  91. kittywhiskers referenced this in commit 992b92383f on Sep 1, 2021
  92. kittywhiskers referenced this in commit 9be98be6d2 on Sep 1, 2021
  93. kittywhiskers referenced this in commit 3c68a9e218 on Sep 2, 2021
  94. kittywhiskers referenced this in commit 8b935211ca on Sep 3, 2021
  95. kittywhiskers referenced this in commit a4369ecc5d on Sep 3, 2021
  96. kittywhiskers referenced this in commit 2f690755f4 on Sep 3, 2021
  97. kittywhiskers referenced this in commit a2687d4594 on Sep 4, 2021
  98. DrahtBot locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me