-
Notifications
You must be signed in to change notification settings - Fork 57
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 CI script to manually build tarballs for the releases. #2228
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2228 +/- ##
=======================================
Coverage 83.64% 83.64%
=======================================
Files 107 107
Lines 23168 23168
=======================================
Hits 19378 19378
Misses 3790 3790 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ni4 ! Left some suggestions for you... :)
ci-legacy/build_tarball.sh
Outdated
|
||
# Check whether tarball builds | ||
# cpack would use symantic versioning for file names, i.e. rnp-v0.17.1 | ||
RNP_TGZ=$(echo ${RNP_BLD}/*.tar.gz) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to enclose variables in double quotes:
-RNP_TGZ=$(echo ${RNP_BLD}/*.tar.gz)
+RNP_TGZ=$(echo "${RNP_BLD}"/*.tar.gz)
Fine for now, but could break the tar
command below if somehow there is more than one .tar.gz
present in the $RNP_BLD
directory.
Quick and dirty 2nd take:
-RNP_TGZ=$(echo ${RNP_BLD}/*.tar.gz)
+RNP_TGZ=$(find "${RNP_BLD}" -maxdepth 1 -type f -name '*.tar.gz' | head -n 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!
ci-legacy/build_tarball.sh
Outdated
mkdir -p "${RNP_CHK}" | ||
|
||
tar -xzf "${RNP_TGZ}" -C "${RNP_CHK}" | ||
RNP_UNP=$(echo ${RNP_CHK}/*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to enclose variables in double quotes:
-RNP_UNP=$(echo ${RNP_CHK}/*)
+RNP_UNP=$(echo "${RNP_CHK}"/*)
or, to prevent from returning multiple directories:
-RNP_UNP=$(echo ${RNP_CHK}/*)
+RNP_UNP=$(find "${RNP_CHK}" -maxdepth 1 -type d | head -n 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!
ci-legacy/build_tarball.sh
Outdated
|
||
tar -xzf "${RNP_TGZ}" -C "${RNP_CHK}" | ||
RNP_UNP=$(echo ${RNP_CHK}/*) | ||
cmake -B ${RNP_CHK}/build -DBUILD_SHARED_LIBS=ON -DBUILD_TESTING=ON "${RNP_UNP}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to enclose variables in double quotes:
-cmake -B ${RNP_CHK}/build -DBUILD_SHARED_LIBS=ON -DBUILD_TESTING=ON "${RNP_UNP}"
+cmake -B "${RNP_CHK}"/build -DBUILD_SHARED_LIBS=ON -DBUILD_TESTING=ON "${RNP_UNP}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!
ci-legacy/build_tarball.sh
Outdated
tar -xzf "${RNP_TGZ}" -C "${RNP_CHK}" | ||
RNP_UNP=$(echo ${RNP_CHK}/*) | ||
cmake -B ${RNP_CHK}/build -DBUILD_SHARED_LIBS=ON -DBUILD_TESTING=ON "${RNP_UNP}" | ||
cmake --build ${RNP_CHK}/build --parallel "$(nproc)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to enclose variables in double quotes:
-cmake --build ${RNP_CHK}/build --parallel "$(nproc)"
+cmake --build "${RNP_CHK}"/build --parallel "$(nproc)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!
ci-legacy/build_tarball.sh
Outdated
# Check whether artifacts dir exists | ||
RNP_ART="/opt/artifacts" | ||
if [ ! -d "${RNP_ART}" ]; then | ||
echo "Error: artifacts dir ${RNP_ART} doesn't exist. Create or mount it before running the script." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to direct error messages to stderr:
-echo "Error: artifacts dir ${RNP_ART} doesn't exist. Create or mount it before running the script."
+>&2 echo "Error: artifacts dir ${RNP_ART} doesn't exist. Create or mount it before running the script."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!
23fafb7
to
6dddbe1
Compare
Thanks, @ribose-jeffreylau ! Fixed and re-pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ni4!
6dddbe1
to
b79a303
Compare
Does it build the source tarball or the binary one? |
There is one failing test... |
It builds (and checks) source tarball, so it could be uploaded to the release artifacts. At some point it would be upgraded to run automatically on tag push or PR merge to the release branch :-)
Thanks, re-run it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale behind ci-legacy
folder was to stash there unused ci scripts and remove this folder eventually (or optionally)
Please consider moving this script to ci
folder
b79a303
to
02bf6ee
Compare
@maxirmx Oh, sure. Thanks for noticing. |
@maxirmx Is it good to merge now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
It could be automated/refactored in some way (like for tag push), however let's have at least manual one for now.
Related to #2227