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

Add shellcheck to CI and a progress bar for sed'ing in genbindings.sh #127

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tnull
Copy link

@tnull tnull commented Jun 6, 2024

As running the C++ sed step is insanely slow, we print a simple progress bar to allow checking if progress is made.

We also run shellcheck on genbindings.sh and enforce it in CI.

@tnull tnull force-pushed the 2024-06-shellcheck-and-progress branch from aa0536e to ac0c537 Compare June 6, 2024 10:12
@tnull tnull force-pushed the 2024-06-shellcheck-and-progress branch from ac0c537 to 4188e53 Compare June 6, 2024 10:22
./a.out
fi
fi
# And run the C++ demo app
Copy link
Collaborator

Choose a reason for hiding this comment

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

The indentation here is wrong now, we're still in a top-level if.

EXTRA_TARGETS=( $LDK_C_BINDINGS_EXTRA_TARGETS )
EXTRA_CCS=( $LDK_C_BINDINGS_EXTRA_TARGET_CCS )
EXTRA_LINK_LTO=( $LDK_C_BINDINGS_EXTRA_TARGET_LINK_LTO )
EXTRA_TARGETS=( "$LDK_C_BINDINGS_EXTRA_TARGETS" )
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, this breaks the array expansion.

fi
# Finally, sanity-check the generated C and C++ bindings with demo apps:
# Naively run the C demo app:
gcc "$LOCAL_CFLAGS" -Wall -g -pthread demo.c target/debug/libldk.a -ldl -lm
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, we want the arguments to expand, quoting restricts them to a single argument.

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.

2 participants