[fix, build] Don't fail quietly on patching failure #895

Frenzie · 2019-04-19T14:14:56Z

It's a vestigial property from before we switched to CMake.

Frenzie · 2019-04-19T14:34:09Z

@NiLuJe Hm, apparently I was wrongish. I'll look into it a little deeper in any case, because this || true is freaking terrible.

-- Build files have been written to: /home/frans/src/kobo/koreader/base/thirdparty/luajit/build/x86_64-linux-gnu-debug
[1/5] Performing patch step for 'luajit'
FAILED: luajit-prefix/src/luajit-stamp/luajit-patch 
cd /home/frans/src/kobo/koreader/base/thirdparty/luajit/build/x86_64-linux-gnu-debug/luajit-prefix/src/luajit && sh -c "patch -N -p1 < /home/frans/src/kobo/koreader/base/thirdparty/luajit/koreader-luajit-makefile-tweaks.patch" && /usr/bin/cmake -E touch /home/frans/src/kobo/koreader/base/thirdparty/luajit/build/x86_64-linux-gnu-debug/luajit-prefix/src/luajit-stamp/luajit-patch
patching file src/Makefile
Reversed (or previously applied) patch detected!  Skipping patch.
4 out of 4 hunks ignored -- saving rejects to file src/Makefile.rej
ninja: build stopped: subcommand failed.

NiLuJe · 2019-04-19T14:36:38Z

Hide it away in a utility function in our cmake bag of tricks?

Would have to take a list for projects with multiple patches and we'd just have to feed the filename?

(Similar to how Gentoo ebuilds use a PATCHES bash array).

Frenzie · 2019-04-19T14:38:33Z

It looks like it might be really easy to handle. The error code for previously applied is 1, and for actual errors it's greater than 1.

NiLuJe · 2019-04-19T14:38:44Z

Oh, wait, that wouldn't solve the "silent failure" issue, though -_-".

EDIT: Ha, too slow ;).

Frenzie · 2019-04-19T14:41:55Z

I think there's an easier solution: --merge.

NiLuJe · 2019-04-19T14:46:00Z

Not supported on macOS ;).

patch: unrecognized option `--merge'

Frenzie · 2019-04-19T14:49:00Z

@NiLuJe Could you check the error codes?

NiLuJe · 2019-04-19T14:50:19Z

In which instances?

(Right now, it return 2 for unsupported option ;)).

Frenzie · 2019-04-19T14:51:01Z

Like I said above, 1 is previously applied, greater than 1 is other (actual) errors.

NiLuJe · 2019-04-19T14:51:28Z

Yep, that holds true on macOS, too.

Frenzie · 2019-04-19T14:51:53Z

Alright, in that case I'll write a simple wrapper script.

It's GNU Patch only. ;-; This reverts commit 93abe3e.

Frenzie · 2019-04-19T15:23:43Z

@NiLuJe Something like this work on Mac OS?

#!/bin/sh

# depends on input from stdin

patch -p1 -N --dry-run --silent 2>/dev/null

exit_status=$?

if [ $exit_status -eq 0 ]; then
    echo "Applying patch."
    patch -p1 -N
    exit $?
elif [ $exit_status -eq 1 ]; then
    echo "Previously applied patch, ignoring."
    exit 0
fi

exit $?

diff --git a/thirdparty/cmake_modules/koreader_thirdparty_common.cmake b/thirdparty/cmake_modules/koreader_thirdparty_common.cmake
index fff09c2..1be801a 100644
--- a/thirdparty/cmake_modules/koreader_thirdparty_common.cmake
+++ b/thirdparty/cmake_modules/koreader_thirdparty_common.cmake
@@ -24,6 +24,8 @@ else()
     set(KO_MAKE_RECURSIVE make)
 endif()
 
+set(KO_PATCH "sh ${CMAKE_MODULE_PATH}/patch-wrapper.sh")
+
 macro(assert_var_defined varName)
     if(NOT DEFINED ${varName})
         message(FATAL_ERROR "${varName} variable not defined!")

NiLuJe · 2019-04-19T17:21:38Z

That looks sane, but I'll double-check when I'm back on the laptop later ;)

Frenzie · 2019-04-19T18:03:40Z

I'll update the PR to include that. The basic strategy seems right even if some minor details in the script itself could need tweaking for Mac OS. This way it seems more elegant to me, but the same thing can always be done without the dry-run part.

PS I think sdcv is working out because it's handled completely by CMake, and the conflicts arise because for whatever reason the patching process is a bit weird for ExternalProject. The CMake source just says that it can be hard to write a good patch command, roflmao.

NiLuJe · 2019-04-19T18:27:18Z

CMake being helpful, as always :D.

Frenzie · 2019-04-19T18:34:35Z

https://github.com/Kitware/CMake/blob/0793464d95c3dbbe9ed87bb5882d29efb7a8e3c3/Modules/ExternalProject.cmake#L352-L358

    ``PATCH_COMMAND <cmd>...``
      Specifies a custom command to patch the sources after an update. By
      default, no patch command is defined. Note that it can be quite difficult
      to define an appropriate patch command that performs robustly, especially
      for download methods such as git where changing the ``GIT_TAG`` will not
      discard changes from a previous patch, but the patch command will be
called again after updating to the new tag.

Frenzie · 2019-04-19T18:50:37Z

@NiLuJe Not 100 % sure, but I think the problem may be localized to BUILD_IN_SOURCE 1. (luajit, mupdf, openssl)

Frenzie · 2019-04-20T06:09:02Z

@NiLuJe In other news, introducing a failure (on purpose here for testing) still returns an exit status of 0 for the OpenSSL configure script? O_o

-- Build files have been written to: /home/frans/src/kobo/koreader-base/thirdparty/openssl/build/x86_64-linux-gnu
[1/4] Performing configure step for 'openssl'
Operating system: x86_64-whatever-linux2

Failure!  build file wasn't produced.
Please read INSTALL and associated NOTES files.  You may also have to look over
your available compiler tool chain or change your configuration.

***** Unsupported options: no-r5
0
[2/4] Performing build step for 'openssl'
make[1]: Entering directory '/home/frans/src/kobo/koreader-base/thirdparty/openssl/build/x86_64-linux-gnu/openssl-prefix/src/openssl'
make[1]: Leaving directory '/home/frans/src/kobo/koreader-base/thirdparty/openssl/build/x86_64-linux-gnu/openssl-prefix/src/openssl'
make[1]: Entering directory '/home/frans/src/kobo/koreader-base/thirdparty/openssl/build/x86_64-linux-gnu/openssl-prefix/src/openssl'
make _build_libs
make[2]: Entering directory '/home/frans/src/kobo/koreader-base/thirdparty/openssl/build/x86_64-linux-gnu/openssl-prefix/src/openssl'
make[2]: Nothing to be done for '_build_libs'.
make[2]: Leaving directory '/home/frans/src/kobo/koreader-base/thirdparty/openssl/build/x86_64-linux-gnu/openssl-prefix/src/openssl'
make[1]: Leaving directory '/home/frans/src/kobo/koreader-base/thirdparty/openssl/build/x86_64-linux-gnu/openssl-prefix/src/openssl'
[4/4] Completed 'openssl'

Frenzie · 2019-04-20T06:52:27Z

Darn, you also get an exit code of 1 for hunks failed to apply. @NiLuJe Does Mac OS have the -b flag?

NB Only returning false success in some instances is still better than always returning success.

Frenzie · 2019-04-20T07:33:49Z

@NiLuJe I think this updated script is pretty fool-proof.

#!/bin/sh

if [ $# -eq 0 ]; then
    echo "Patch file required as argument."
    exit 1
fi

PATCH_FILE=$1

# Reverse patch will succeed if the patch is already applied.
# In case of failure, it means we should try to apply the patch.
if ! patch -R -p1 -N --dry-run <"${PATCH_FILE}" >/dev/null 2>&1; then
    # Now patch for real.
    patch -p1 -N <"${PATCH_FILE}"
    exit $?
fi

So if the reverse patch doesn't succeed, try to patch, and return the exit status. If the reverse patch succeeds, it means the patch was already successfully applied previously.

koreader/koreader-base#896 * Bump HarfBuzz to 2.4.0 * Bump K2pdfopt to pickup the CJK thingy * Bump libpng to 1.6.37 * Bump FBInk to 1.15.0 [fix, build] Don't fail quietly on patching failure koreader/koreader-base#895

NiLuJe · 2019-04-20T13:15:41Z

Yeah, the reverse test should do the job. I'd check what happens with patches that add completely new files though, because I have a vague memory of trying something like that somewhere and this specific case being weird...

…

On Sat, Apr 20, 2019, 14:31 Frans de Jonge ***@***.***> wrote: Merged #895 <#895> into master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#895 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA3KZQC5PYWZEYFRTVI6SDPRMEI7ANCNFSM4HHEK5EQ> .

koreader/koreader-base#896 * Bump HarfBuzz to 2.4.0 * Bump K2pdfopt to pickup the CJK thingy * Bump libpng to 1.6.37 * Bump FBInk to 1.15.0 [fix, build] Don't fail quietly on patching failure koreader/koreader-base#895

houqp · 2019-04-21T01:51:20Z

nice work on the patch wrapper 👍

koreader/koreader-base#896 * Bump HarfBuzz to 2.4.0 * Bump K2pdfopt to pickup the CJK thingy * Bump libpng to 1.6.37 * Bump FBInk to 1.15.0 [fix, build] Don't fail quietly on patching failure koreader/koreader-base#895

[fix, build] Don't fail quietly on patch fail

95cd6d9

See koreader#892 (comment)

Frenzie added the bug label Apr 19, 2019

Frenzie changed the title ~~[fix, build] Don't fail quietly on patch fail~~ [fix, build] Don't fail quietly on patching failure Apr 19, 2019

beauty space

8b459c1

Use --merge

93abe3e

Revert "Use --merge"

bcd82d3

It's GNU Patch only. ;-; This reverts commit 93abe3e.

Frenzie added 3 commits April 20, 2019 08:30

use patch wrapper

cc9cc4c

and the rest

dc56848

also sdcv

d1bd62c

Frenzie added 2 commits April 20, 2019 09:27

try to dry run reverse first

0eee0b9

update syntax

7cebe31

Frenzie merged commit ec77fe8 into koreader:master Apr 20, 2019

Frenzie deleted the fail-patches branch April 20, 2019 12:31

Frenzie mentioned this pull request Apr 20, 2019

bump base to bump some dependencies koreader/koreader#4948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix, build] Don't fail quietly on patching failure #895

[fix, build] Don't fail quietly on patching failure #895

Frenzie commented Apr 19, 2019 •

edited

Loading

Frenzie commented Apr 19, 2019

NiLuJe commented Apr 19, 2019 •

edited

Loading

Frenzie commented Apr 19, 2019

NiLuJe commented Apr 19, 2019 •

edited

Loading

Frenzie commented Apr 19, 2019

NiLuJe commented Apr 19, 2019

Frenzie commented Apr 19, 2019

NiLuJe commented Apr 19, 2019

Frenzie commented Apr 19, 2019

NiLuJe commented Apr 19, 2019

Frenzie commented Apr 19, 2019

Frenzie commented Apr 19, 2019 •

edited

Loading

NiLuJe commented Apr 19, 2019

Frenzie commented Apr 19, 2019 •

edited

Loading

NiLuJe commented Apr 19, 2019

Frenzie commented Apr 19, 2019

Frenzie commented Apr 19, 2019

Frenzie commented Apr 20, 2019

Frenzie commented Apr 20, 2019 •

edited

Loading

Frenzie commented Apr 20, 2019

NiLuJe commented Apr 20, 2019 via email •

edited

Loading

houqp commented Apr 21, 2019

[fix, build] Don't fail quietly on patching failure #895

[fix, build] Don't fail quietly on patching failure #895

Conversation

Frenzie commented Apr 19, 2019 • edited Loading

Frenzie commented Apr 19, 2019

NiLuJe commented Apr 19, 2019 • edited Loading

Frenzie commented Apr 19, 2019

NiLuJe commented Apr 19, 2019 • edited Loading

Frenzie commented Apr 19, 2019

NiLuJe commented Apr 19, 2019

Frenzie commented Apr 19, 2019

NiLuJe commented Apr 19, 2019

Frenzie commented Apr 19, 2019

NiLuJe commented Apr 19, 2019

Frenzie commented Apr 19, 2019

Frenzie commented Apr 19, 2019 • edited Loading

NiLuJe commented Apr 19, 2019

Frenzie commented Apr 19, 2019 • edited Loading

NiLuJe commented Apr 19, 2019

Frenzie commented Apr 19, 2019

Frenzie commented Apr 19, 2019

Frenzie commented Apr 20, 2019

Frenzie commented Apr 20, 2019 • edited Loading

Frenzie commented Apr 20, 2019

NiLuJe commented Apr 20, 2019 via email • edited Loading

houqp commented Apr 21, 2019

Frenzie commented Apr 19, 2019 •

edited

Loading

NiLuJe commented Apr 19, 2019 •

edited

Loading

NiLuJe commented Apr 19, 2019 •

edited

Loading

Frenzie commented Apr 19, 2019 •

edited

Loading

Frenzie commented Apr 19, 2019 •

edited

Loading

Frenzie commented Apr 20, 2019 •

edited

Loading

NiLuJe commented Apr 20, 2019 via email •

edited

Loading