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

ci: Add a clang-ast workflow #1480

Merged
merged 1 commit into from
Oct 31, 2024
Merged

ci: Add a clang-ast workflow #1480

merged 1 commit into from
Oct 31, 2024

Conversation

ac000
Copy link
Member

@ac000 ac000 commented Oct 25, 2024

    This does compile-time type and argument checking using a clang-plugin.
    It was run as part of buildbot.
    
    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]>

@ac000 ac000 force-pushed the ci-clang-ast branch 2 times, most recently from a4896da to 2b671cd Compare October 25, 2024 16:58
@thresheek
Copy link
Member

thresheek commented Oct 25, 2024

@thresheek
Copy link
Member

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

@ac000 ac000 force-pushed the ci-clang-ast branch 4 times, most recently from d51aad5 to 83557d9 Compare October 25, 2024 17:20
@ac000
Copy link
Member Author

ac000 commented Oct 25, 2024

PR opened.

@ac000
Copy link
Member Author

ac000 commented Oct 25, 2024

  • 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 ac000 marked this pull request as ready for review October 25, 2024 18:03
Copy link
Contributor

@avahahn avahahn left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@avahahn avahahn left a comment

Choose a reason for hiding this comment

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

Can you clarify the desired outcome to this PR? 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.

- name: Build Unit
run: |
make -j4 \
EXTRA_CFLAGS="-Xclang -load -Xclang clang-ast/ngx-ast.so -Xclang -plugin -Xclang ngx-ast" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be split across multiple lines to adhere to our repository wide 80 character line length limit?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think so.

As you can see, I've split long lines where it makes sense.

This is no different to the other workflow files or the README for example.

@ac000
Copy link
Member Author

ac000 commented Oct 26, 2024

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
Copy link
Member Author

ac000 commented Oct 26, 2024

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
Copy link
Member Author

ac000 commented Oct 26, 2024

  • 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 ac000 force-pushed the ci-clang-ast branch 2 times, most recently from 0893f6b to 107055e Compare October 26, 2024 02:47
@ac000
Copy link
Member Author

ac000 commented Oct 26, 2024

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 ac000 force-pushed the ci-clang-ast branch 5 times, most recently from d5193e3 to f04bc68 Compare October 26, 2024 03:22
@ac000
Copy link
Member Author

ac000 commented Oct 26, 2024

  • 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 ac000 force-pushed the ci-clang-ast branch 2 times, most recently from 2f0976e to a24009a Compare October 26, 2024 03:57
@ac000 ac000 force-pushed the ci-clang-ast branch 2 times, most recently from 09fa063 to 37c9fdd Compare October 26, 2024 04:08
@ac000 ac000 marked this pull request as draft October 26, 2024 04:09
@ac000 ac000 force-pushed the ci-clang-ast branch 11 times, most recently from 3b2d79d to 9fcf602 Compare October 26, 2024 17:26
@ac000
Copy link
Member Author

ac000 commented Oct 26, 2024

  • 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
Copy link
Member Author

ac000 commented Oct 26, 2024

$ 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

This does compile-time type and argument checking using a clang-plugin.
It was run as part of buildbot.

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]>
@ac000
Copy link
Member Author

ac000 commented Oct 29, 2024

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 ac000 marked this pull request as ready for review October 29, 2024 01:44
@ac000 ac000 merged commit 1e345b3 into nginx:master Oct 31, 2024
2 checks passed
@ac000 ac000 deleted the ci-clang-ast branch October 31, 2024 00:38
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.

4 participants