Create an option to skip tests #119
Conversation
genbindings.sh
Outdated
| SKIP_TESTS=false | ||
|
|
||
| if [ ! -z "$SKIP_TESTS_ARGUMENT" ]; then | ||
| if [ "$SKIP_TESTS_ARGUMENT" != "skip" ]; then |
There was a problem hiding this comment.
Maybe call it skip-tests?
arik-so
force-pushed
the
arik/2024/01/skip-tests-option
branch
from
7c1b133 to
0ba47de
Compare
genbindings.sh
Outdated
| @@ -249,6 +261,11 @@ else | |||
| sed -i '' 's/#include <stdlib.h>/#include "ldk_rust_types.h"/g' include/lightning.h | |||
| fi | |||
|
|
|||
| if $SKIP_TESTS; then | |||
| echo "Skipping tests!" | |||
| exit 0 | |||
There was a problem hiding this comment.
At this point we aren't just at the test step, but we still haven't built the headers or the binaries. We should instead just skip the cases where we build/run demo.{c,cpp}/a.out.
There was a problem hiding this comment.
Hm, are you talking about the CPP headers and binaries? Because the C headers are already built. This is where the really long wait starts on Mac when not using GNU sed. So perhaps I should have both a skip-tests and a skip-cpp option, with the latter being more descriptive of this location.
There was a problem hiding this comment.
Yes, C++ headers, but also all the cargo build calls need to happen so that we have binaries in place for downstream language bindings to use.
There was a problem hiding this comment.
It would still be nice to have a command to skip at this step though, at the very least for Swift bindings, because there are scripts downstream that generate the binaries for whichever iOS chip architecture is being built by Xcode. That could minimize some build duplication.
There was a problem hiding this comment.
Why not use LDK_C_BINDINGS_EXTRA_TARGETS/LDK_C_BINDINGS_EXTRA_TARGET_CCS to get it built in genbindings.sh with the right RUSTFLAGS set for you for determinism and path-hiding?
There was a problem hiding this comment.
I guess we could. Currently, Xcode automatically specifies the target it needs for a given configuration, and there's a build script that simply passes it through, which allows for more parallelizable CI actions.
I guess it has a minor benefit that when Apple adds some weird new architecture or device, Xcode automatically provides the necessary target, but really it's mostly a vestige of how the project architecture grew dynamically.
There was a problem hiding this comment.
Yea, I mean I'd substantially prefer using the genbindings.sh script bins, if only because it makes determinism one step closer (and maybe already there?). Can you update this PR to just skip the tests themselves and then look into using the built binaries for your builds?
There was a problem hiding this comment.
It's the execution of the C++ binaries that crashes on Mac CIs, so I don't think I can really run any commands downstream from where the interruption occurs. Given the binary is largely a sanity check and executes beautifully on Linux, I'd rather not hold up the Swift bindings release until we fix the underlying bus error. I also think once it's fixed, the actual tests will comprise so insignificant a proportion of the total execution time that the option to skip them would become moot. Open to argument rename suggestions.
There was a problem hiding this comment.
Right, the C++ binaries are all tests, we can skip all of those calls but still build with cargo build/cargo rustc.
checking and running the gcc app.
|
LGTM. I think there's one or two more demo app builds at the end, but we can fix that later (in one commit next time, please :p). |
I opted to make the
skipoption hidden so as not to advertise it, but am happy to adjust.