Add GitHub Actions workflow for JSHooks testing source#449
Add GitHub Actions workflow for JSHooks testing source#449tequdev wants to merge 4 commits intoXahau:jshooksfrom
Conversation
- Create a new GitHub Actions workflow for testing JSHooks sources - Update script to generate SetJSHook_wasm.h
| - name: Install dependencies | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y build-essential clang-format bc |
There was a problem hiding this comment.
Maybe we should pin a specific version of clang-format in some environment variable that be easily changed.
I noticed the other day that clang-format@19 would not work for some files.
There was a problem hiding this comment.
It seems that clang-format-10 is not provided in ubuntu-latest (ubuntu 22.04).
We can install clang-format-10 in ubuntu 20.04, but errors will occur around qjsc.
At the moment, there is no effect on SetJSHook_wasm.h, so I will leave it as it is.
If you could fix it, I would be very grateful.
There was a problem hiding this comment.
I started working on extracting an action in #450
I'll make a PR to this once that's sorted we can just use that
There was a problem hiding this comment.
@dangell7 wasn't a fan of touching the rippled shared clang-format workflow, in anticipation of merges, but I guess given this PR and #440 both install clang-format, we could use this action:
name: Install Clang-Format Ubuntu
description: Installs a specific version of clang-format on Ubuntu 20.04 and newer
inputs:
clang-version:
description: "Version of Clang-Format to install"
required: true
default: "10"
runs:
using: "composite"
steps:
- name: Install Clang-Format
run: |
codename=$(lsb_release --codename --short)
# Use modern key management for Ubuntu 20.04+ compatibility
wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo tee /usr/share/keyrings/llvm-archive-keyring.gpg >/dev/null
# Update LLVM repository sources
echo "deb [signed-by=/usr/share/keyrings/llvm-archive-keyring.gpg] http://apt.llvm.org/${codename}/ llvm-toolchain-${codename}-${{ inputs.clang-version }} main" | sudo tee /etc/apt/sources.list.d/llvm.list
echo "deb-src [signed-by=/usr/share/keyrings/llvm-archive-keyring.gpg] http://apt.llvm.org/${codename}/ llvm-toolchain-${codename}-${{ inputs.clang-version }} main" | sudo tee -a /etc/apt/sources.list.d/llvm.list
# Update and install Clang-Format
sudo apt-get update
sudo apt-get install -y clang-format-${{ inputs.clang-version }}
# Verify installation
clang-format-${{ inputs.clang-version }} --version
shell: bash
There was a problem hiding this comment.
For the record, this is using clang-format 18 which seems to be fine for the file formatted by this.
src/test/app/build_test_jshooks.sh
Outdated
| # | ||
| # - clang-format: | ||
| # Ubuntu: $sudo apt-get install clang-format | ||
| # macOS: $brew install clang-format |
There was a problem hiding this comment.
brew install r-lib/taps/clang-format@10
To match the CI clang-format I had to use that. 19 wasn't working in every case.
related to:
https://github.com/Xahau/xahaud/pull/449/files#r1972938606
sublimator
left a comment
sublimator
left a comment
There was a problem hiding this comment.
Other than the ggrep -> grep and gsed -> sed changes, this looks good to me. I think I'd rather leave the macos defaults as they are for other scripts?
|
We better double check me re: cf 10
I may be confused
…On Thu, 27 Feb 2025, 6:20 pm tequ, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .github/workflows/js_hooks_test.yml
<#449 (comment)>:
> +concurrency:
+ group: ${{ github.workflow }}
+ cancel-in-progress: false
+
+jobs:
+ jshooks-test-sources:
+ runs-on: ubuntu-latest
+
+ steps:
+ - name: Checkout
+ uses: ***@***.***
+
+ - name: Install dependencies
+ run: |
+ sudo apt-get update
+ sudo apt-get install -y build-essential clang-format bc
It seems that clang-format-10 is not provided in ubuntu-latest (ubuntu
22.04).
We can install clang-format-10 in ubuntu 20.04, but errors will occur
around qjsc.
At the moment, there is no effect on SetJSHook_wasm.h, so I will leave it
as it is.
If you could fix it, I would be very grateful.
—
Reply to this email directly, view it on GitHub
<#449 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEAHGY6GNJ2PHQ7T5PF7X32R3YHTAVCNFSM6AAAAABX2PZO2KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNBXGQ2TAMBRGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
I'm not sure how valuable the workflow is. We barely touch the test file once its merged. Right now, you will know its different because of the merge conflict. I approve the bash changes but I really dont think the CI is necessary to run every single time when it will be touched so little. |
I will create a separate script-only PR. |
I'll say that yesterday I forgot to update the wasm in one of the PRs, and then it seems one of the tests failed because of that. Of course the beast unit test framework, for some reason just prints that there was 1 failure rather than helpfully listing WHICH test actually failed and at what file/line. This workflow would have helped me more quickly recognize what the issue was. I don't think it's strictly necessary either, but it's definitely a small quality of life improvement, and you can set |
|
I must note that I've successfully brainwashed myself into referring to the qjsc as "wasm" :) |
|
Closing in favour of #464 |
3 participants
High Level Overview of Change
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)