unit icon indicating copy to clipboard operation
unit copied to clipboard

ci: Add a clang-ast workflow

Open ac000 opened this issue 1 year ago • 9 comments

ac000 avatar Oct 25 '24 16:10 ac000

FYI we have https://github.com/nginx/clang-ast/tree/unit

thresheek avatar Oct 25 '24 17:10 thresheek

Ah I see you have forked this repo :) Maybe shoot a PR for changes so we keep it in one place.

thresheek avatar Oct 25 '24 17:10 thresheek

PR opened.

ac000 avatar Oct 25 '24 17:10 ac000

  • Switch to the unit branch of nginx/clang-ast now that the needed fix has been merged, thanks to @thresheek
 $ git range-diff 83557d99...3304aaf4
1:  83557d99 ! 1:  3304aaf4 ci: Add a clang-ast workflow
    @@ .github/workflows/clang-ast.yaml (new)
     +
     +      - name: Checkout and build clang-ast
     +        run: |
    -+          git clone https://github.com/ac000/clang-ast -b unit-stdc++17
    ++          git clone https://github.com/nginx/clang-ast -b unit
     +          cd clang-ast
     +          make
     +

ac000 avatar Oct 25 '24 18:10 ac000

Should test be included in this check? it's mostly Python code, and I am not aware of any difference it could make in the output Unit binary

The pytests have no bearing on this.

However we do have the C tests as enabled with --tests.

I was in two minds whether to include them or not.

Having tried running the plugin on the tests, it did actually find a problem (in the test code)

src/test/nxt_base64_test.c:90:13: error: number of arguments for nxt_log_s::handler mismatches query string 'nxt_base64_decode() test "%V" failed', expected 1 args, got 0 args
   90 |             nxt_log_alert(thr->log, "nxt_base64_decode() test \"%V\" failed");
      |             ^
src/nxt_log.h:81:9: note: expanded from macro 'nxt_log_alert'
   81 |         _log_->handler(NXT_LOG_ALERT, _log_, __VA_ARGS__);                    \
      |         ^
1 error generated.

So, OK, let's include them..

It could also be expanded out to include language modules...

ac000 avatar Oct 26 '24 02:10 ac000

Can you clarify the desired outcome to this PR?

This was a check that used to run on our buildbot infrastructure, this is simply running it here now.

I am not sure if this is supposed to be a solution to the code formatting or if this is to check some aspect of the build in each branch.

This has diddly-squat to do with code-formatting...

ac000 avatar Oct 26 '24 02:10 ac000

  • Run this over the C tests also
  • We need to use -add-plugin in order to have the object files created
  • Add a fix for an issue it caught in the tests
$ git range-diff 3304aaf4...dad4727a
-:  -------- > 1:  cfb52003 src/test: Fix missing parameter to nxt_log_alert() in nxt_base64_test()
1:  3304aaf4 ! 2:  dad4727a ci: Add a clang-ast workflow
    @@ .github/workflows/clang-ast.yaml (new)
     +
     +      - name: Checkout and build clang-ast
     +        run: |
    -+          git clone https://github.com/nginx/clang-ast -b unit
    ++          git clone https://github.com/nginx/clang-ast.git -b unit
     +          cd clang-ast
     +          make
     +
    @@ .github/workflows/clang-ast.yaml (new)
     +      - name: Build Unit
     +        run: |
     +          make -j4 \
    -+            EXTRA_CFLAGS="-Xclang -load -Xclang clang-ast/ngx-ast.so -Xclang -plugin -Xclang ngx-ast" \
    ++            EXTRA_CFLAGS="-Xclang -load -Xclang clang-ast/ngx-ast.so -Xclang -add-plugin -Xclang ngx-ast" \
     +            NXT_SHARED_LOCAL_LINK=: build/lib/libnxt.so
    ++
    ++      - name: Build C tests
    ++        run: |
    ++          EXTRA_CFLAGS="-Xclang -load -Xclang clang-ast/ngx-ast.so -Xclang -add-plugin -Xclang ngx-ast" \
    ++          tests

ac000 avatar Oct 26 '24 02:10 ac000

Need to actually configure and build the tests!

$ git range-diff dad4727a...107055ed
1:  dad4727a ! 1:  107055ed ci: Add a clang-ast workflow
    @@ .github/workflows/clang-ast.yaml (new)
     +          make
     +
     +      - name: Configure Unit
    -+        run: ./configure --cc=clang --openssl --debug
    ++        run: ./configure --cc=clang --openssl --debug --tests
     +
     +      - name: Build Unit
     +        run: |
    @@ .github/workflows/clang-ast.yaml (new)
     +
     +      - name: Build C tests
     +        run: |
    -+          EXTRA_CFLAGS="-Xclang -load -Xclang clang-ast/ngx-ast.so -Xclang -add-plugin -Xclang ngx-ast" \
    -+          tests
    ++          make -j4 \
    ++            EXTRA_CFLAGS="-Xclang -load -Xclang clang-ast/ngx-ast.so -Xclang -add-plugin -Xclang ngx-ast" \
    ++            tests

ac000 avatar Oct 26 '24 02:10 ac000

  • Set the clang-ast plugin arguments at configure time
$ git range-diff 107055e...f04bc68d
1:  107055ed ! 1:  f04bc68d ci: Add a clang-ast workflow
    @@ .github/workflows/clang-ast.yaml (new)
     +          make
     +
     +      - name: Configure Unit
    -+        run: ./configure --cc=clang --openssl --debug --tests
    ++        run: ./configure --cc=clang --cc-opt="-Xclang -load -Xclang clang-ast/ngx-ast.so -Xclang -add-plugin -Xclang ngx-ast" --openssl --debug --tests
     +
     +      - name: Build Unit
     +        run: |
    -+          make -j4 \
    -+            EXTRA_CFLAGS="-Xclang -load -Xclang clang-ast/ngx-ast.so -Xclang -add-plugin -Xclang ngx-ast" \
    -+            NXT_SHARED_LOCAL_LINK=: build/lib/libnxt.so
    ++          make -j4 NXT_SHARED_LOCAL_LINK=: build/lib/libnxt.so
     +
     +      - name: Build C tests
    -+        run: |
    -+          make -j4 \
    -+            EXTRA_CFLAGS="-Xclang -load -Xclang clang-ast/ngx-ast.so -Xclang -add-plugin -Xclang ngx-ast" \
    -+            tests
    ++        run: make -j4 tests

ac000 avatar Oct 26 '24 03:10 ac000

  • Add coverage for the language modules (except go and wasm-wasi-component, neither of which are using clang)
$ git range-diff f04bc68d...9fcf6021
1:  f04bc68d ! 1:  9fcf6021 ci: Add a clang-ast workflow
    @@ Metadata
      ## Commit message ##
         ci: Add a clang-ast workflow
     
    -    This does compile time type-checking using a clang-plugin. It was run as
    -    part of buildbot.
    +    This does compile-time type and argument checking using a clang-plugin.
    +    It was run as part of buildbot.
     
    -    Link: <https://github.com/ac000/clang-ast/tree/unit-stdc%2B%2B17>
    +    This covers unitd, src/test and the php, perl, python, ruby, wasm, java
    +    and nodejs language modules/support.
    +
    +    It doesn't cover Go as that doesn't build anything with clang (uses
    +    cgo) or wasm-wasi-component as that uses rustc.
    +
    +    Link: <https://github.com/nginx/clang-ast/tree/unit>
         Signed-off-by: Andrew Clayton <[email protected]>
     
      ## .github/workflows/clang-ast.yaml (new) ##
    @@ .github/workflows/clang-ast.yaml (new)
     +      - name: Install tools/deps
     +        run: |
     +          apt-get -y update
    -+          apt-get -y install git llvm-dev libclang-dev clang make \
    -+                             libssl-dev libpcre2-dev
    ++          apt-get -y install git wget curl llvm-dev libclang-dev clang make \
    ++                             libssl-dev libpcre2-dev libperl-dev \
    ++                             libphp-embed php-dev python3-dev libpython3-dev \
    ++                             ruby-dev openjdk-17-jdk npm
    ++          npm install -g node-gyp
     +
     +      - uses: actions/checkout@v4
     +
    @@ .github/workflows/clang-ast.yaml (new)
     +        run: ./configure --cc=clang --cc-opt="-Xclang -load -Xclang clang-ast/ngx-ast.so -Xclang -add-plugin -Xclang ngx-ast" --openssl --debug --tests
     +
     +      - name: Build Unit
    -+        run: |
    -+          make -j4 NXT_SHARED_LOCAL_LINK=: build/lib/libnxt.so
    ++        run: make -j4 unitd
     +
     +      - name: Build C tests
     +        run: make -j4 tests
    ++
    ++      - name: Build Perl language module
    ++        run: ./configure perl && make -j4 perl
    ++
    ++      - name: Build PHP language module
    ++        run: ./configure php && make -j4 php
    ++
    ++      - name: Build Python language module
    ++        run: ./configure python --config=python3-config && make -j4 python3
    ++
    ++      - name: Build Ruby language module
    ++        run: ./configure ruby && make -j4 ruby
    ++
    ++      - name: Build Java support
    ++        run: ./configure java && make -j4 java
    ++
    ++      - name: Build Nodejs support
    ++        run: ./configure nodejs && make node-local-install DESTDIR=node
    ++
    ++      - name: Build wasm language module
    ++        run: |
    ++          wget -q -O- https://github.com/bytecodealliance/wasmtime/releases/download/v26.0.0/wasmtime-v26.0.0-x86_64-linux-c-api.tar.xz | tar -xJf -
    ++          ./configure wasm --include-path=wasmtime-v26.0.0-x86_64-linux-c-api/include --lib-path=wasmtime-v26.0.0-x86_64-linux-c-api/lib --rpath && make wasm

ac000 avatar Oct 26 '24 17:10 ac000

  • Drop the src/test fix, that will go in via https://github.com/nginx/unit/pull/1481
$ git range-diff 9fcf6021...c9181663
1:  cfb52003 < -:  -------- src/test: Fix missing parameter to nxt_log_alert() in nxt_base64_test()
2:  9fcf6021 = 1:  c9181663 ci: Add a clang-ast workflow

ac000 avatar Oct 26 '24 18:10 ac000

Rebase with master

$ git range-diff c9181663...1e345b34
-:  -------- > 1:  85f21b7c Use nxt_nitems() instead of sizeof() for strings (arrays)
-:  -------- > 2:  9f6f4866 src/test: Fix missing parameter to nxt_log_alert() in nxt_base64_test()
1:  c9181663 = 3:  1e345b34 ci: Add a clang-ast workflow

ac000 avatar Oct 29 '24 01:10 ac000