chore: add ci and fix some trivial problem - #3
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @FoodChain1028, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've introduced a new CI workflow using GitHub Actions to automate builds for macOS and Linux, targeting both aarch64 and x86_64 architectures. This PR also updates the build system to incorporate zlib as a new dependency and cleans up the codebase by removing an unused source file.
Highlights
- CI Workflow Setup: A new GitHub Actions workflow (.github/workflows/ci.yml) has been added to automate builds for macOS and Linux across aarch64 and x86_64 architectures. It includes steps for Zig setup, dependency caching, formatting checks, and project building. Unit testing is currently a placeholder.
- Build System Updates: The build.zig file has been updated to include zlib as a dependency, specifying its target and optimization settings. The build script now links the zlib library and adds its source path to the build configuration.
- Codebase Simplification: The unused file lsquic_global.c has been removed from the lsquic_files array in build.zig, streamlining the build process.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a CI workflow and adds zlib as a dependency. The changes to the build system are mostly correct, but there is a critical issue with the include path for the new zlib dependency which will prevent the project from building. I've provided a suggestion to fix this. The removal of the unused lsquic_global.c file is a good cleanup.
| lib.linkLibrary(ssl); | ||
| lib.linkLibrary(crypto); | ||
| lib.linkLibrary(zlib.artifact("z")); | ||
| lib.addIncludePath(zlib.path("src")); |
There was a problem hiding this comment.
The include path for zlib appears to be incorrect. The specified zlib dependency (allyourcodebase/zlib) has its header files (zlib.h, zconf.h) at the root of the project, not within a src/ directory. This will likely cause the build to fail when it cannot locate the headers.
To fix this, you should point to the root of the dependency.
lib.addIncludePath(zlib.path(""));
There was a problem hiding this comment.
I am not sure about this, since if there's no these lines in build.zig, the local container building would fail. @GrapeBaBa @spiral-ladder
There was a problem hiding this comment.
you mean lib.addIncludePath(zlib.path("src")); this line?
There was a problem hiding this comment.
am not sure about this, since if there's no these lines in build.zig, the local container building would fail
What is the error message?
| # run: zig build test -Dtarget=${{matrix.target}} --summary all | ||
|
|
||
| - name: Building | ||
| run: zig build -Dtarget=${{matrix.target}} -Doptimize=ReleaseFast --summary all |
There was a problem hiding this comment.
| run: zig build -Dtarget=${{matrix.target}} -Doptimize=ReleaseFast --summary all | |
| run: zig build -Dtarget=${{matrix.target}} -Doptimize=ReleaseSafe --summary all |
Let's try to default to ReleaseSafe rather than ReleaseFast
| # TODO: uncomment when tests are implemented | ||
| # - name: Unit testing | ||
| # run: zig build test -Dtarget=${{matrix.target}} --summary all |
There was a problem hiding this comment.
| # TODO: uncomment when tests are implemented | |
| # - name: Unit testing | |
| # run: zig build test -Dtarget=${{matrix.target}} --summary all |
I think we can do without these if it's going to be commented out anyway.
| - name: Cache Zig installation | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: ${{ steps.setup-zig.outputs.install-path }} | ||
| key: ${{ runner.os }}-zig-${{ env.ZIG_VERSION }} | ||
|
|
||
| - name: Cache Zig build artifacts | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: | | ||
| zig-cache | ||
| ~/.cache/zig | ||
| key: ${{ runner.os }}-${{ matrix.target }}-zig-build-${{ hashFiles('build.zig', 'build.zig.zon') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-${{ matrix.target }}-zig-build- |
There was a problem hiding this comment.
| - name: Cache Zig installation | |
| uses: actions/cache@v3 | |
| with: | |
| path: ${{ steps.setup-zig.outputs.install-path }} | |
| key: ${{ runner.os }}-zig-${{ env.ZIG_VERSION }} | |
| - name: Cache Zig build artifacts | |
| uses: actions/cache@v3 | |
| with: | |
| path: | | |
| zig-cache | |
| ~/.cache/zig | |
| key: ${{ runner.os }}-${{ matrix.target }}-zig-build-${{ hashFiles('build.zig', 'build.zig.zon') }} | |
| restore-keys: | | |
| ${{ runner.os }}-${{ matrix.target }}-zig-build- |
I think for such a small build, we don't really need to cache these.
| - os: ubuntu-latest | ||
| target: aarch64-linux-gnu | ||
| - os: ubuntu-latest | ||
| target: x86_64-linux-gnu |
There was a problem hiding this comment.
Could we try to add a windows target to test
3 participants
This pull request introduces a new CI workflow for building and testing the project, along with updates to the build system to include a new
zlibdependency. Below are the most important changes:close #2
CI Workflow Setup:
.github/workflows/ci.ymlto automate builds for macOS and Linux, targeting bothaarch64andx86_64architectures. It includes steps for setting up Zig, caching dependencies, formatting checks, and building the project. Unit testing is currently commented out as a placeholder for future implementation.Build System Updates:
build.zigfile to includezlibas a dependency, specifying its target and optimization settings.zliblibrary and include its source path in the build configuration.Codebase Simplification:
lsquic_global.cfrom thelsquic_filesarray inbuild.zig.