fix cffi build #2097
Merged
fix cffi build #2097
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 96ca571 in 1 minute and 25 seconds. Click for details.
- Reviewed
29lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:12
- Draft comment:
New push trigger for 'protoc-fix' added. Confirm that filtering on this branch in a reusable workflow is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Reusable workflows are meant to be called by other workflows, not triggered directly. Adding a push trigger to a reusable workflow is unusual and could indicate a mistake. However, the comment just asks for confirmation rather than pointing out a specific issue. The comment violates our rule about not asking for confirmations or intentions. Maybe this is actually a serious architectural issue that deserves attention, even if the comment is phrased as a question? While the underlying concern may be valid, the comment as written doesn't provide actionable feedback - it just asks for confirmation. If there's a real issue, it should be stated directly. Delete the comment. While there may be a legitimate concern about mixing workflow_call with direct triggers, the comment violates our rule about asking for confirmations rather than stating issues directly.
Workflow ID: wflow_Z3HWPmLyEzGKZDlN
You can customize 
| @@ -69,6 +71,13 @@ jobs: | |||
| # shell: bash | |||
| # run: | | |||
| # ci/ubuntu-install-packages | |||
|
|
|||
| - uses: actions/setup-go@v5 | |||
There was a problem hiding this comment.
Duplicate Go setup and protoc-gen-go installation steps detected. Consider consolidating these to avoid redundancy and ensure consistent Go version usage.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 285e713 in 1 minute and 28 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_tnJLUqiwwDK8Ij0H
You can customize
|
|
||
| - uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: '1.24.4' |
There was a problem hiding this comment.
Explicitly setting go-version (1.24.4) improves reproducibility. However, note that a later setup-go step uses go-version '1.21'. Ensure this discrepancy is intentional.
Suggested change
| go-version: '1.24.4' | |
| go-version: '1.21' |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5d139ec in 1 minute and 7 seconds. Click for details.
- Reviewed
27lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:125
- Draft comment:
Remove commented-out 'Build CLI Binary' step if it's unused, or add a brief comment explaining why it's disabled. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While documenting commented-out code can be helpful, this is a CI workflow file where the changes are likely temporary or part of a transition. The file still builds the CLI binary in other steps (evident from the artifact upload steps), so this is likely just a refactor in progress. The comment doesn't point out a clear issue that needs fixing. The commented code could be confusing to future developers, and having an explanation could help. Maybe there's an important reason this was commented out that should be documented. While documentation is good, this comment doesn't meet our bar of "clearly a code change required". It's a nice-to-have suggestion rather than a clear issue. Delete the comment as it doesn't identify a clear issue that needs fixing, and is more of a style suggestion.
Workflow ID: wflow_gCnVZPlo00spjKxw
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 42b3a05 in 2 minutes and 6 seconds. Click for details.
- Reviewed
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:72
- Draft comment:
Removed setup-go step with go-version '1.24.4' but later the workflow uses go-version '1.21'. Verify that this intentional version downgrade won’t affect dependencies. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%The comment is pointing out a potential issue with a version downgrade in the workflow, which could affect dependencies. However, it asks the author to verify the intention, which is against the rules. The comment could be rephrased to suggest checking the impact of the downgrade without asking for verification.
Workflow ID: wflow_SUpybvxFUhI9AmrT
You can customize
|
|
||
| - name: Add protoc-gen-go to path | ||
| run: | | ||
| echo 'export PATH=$PATH:$HOME/go/bin' >> $GITHUB_ENV |
There was a problem hiding this comment.
Avoid using 'export PATH=...' in GITHUB_ENV/GITHUB_PATH. Instead, echo just the directory (e.g. echo "$HOME/go/bin" >> $GITHUB_PATH) per GitHub Actions docs.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 99578d3 in 1 minute and 8 seconds. Click for details.
- Reviewed
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:90
- Draft comment:
If the protoc-gen-go version check is no longer needed, remove the commented-out code to reduce clutter. If you intend to keep it for debugging, add a clarifying note. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is about code cleanliness and maintainability, which is generally good. The suggestion to either remove or document commented code is a reasonable best practice. However, this is a very minor issue in a CI workflow file, and the commented code may be intentionally kept for debugging purposes. The comment may be too nitpicky for a CI configuration file where keeping commented debugging steps is fairly common practice. Also, this kind of minor cleanup suggestion may not be worth the reviewer's attention. While the comment is technically correct, CI files often keep commented sections for quick debugging toggles, and this isn't causing any real maintenance burden. This comment should be removed as it's too minor of an issue for a CI configuration file and the commented code may serve a legitimate debugging purpose.
Workflow ID: wflow_aysGOQJYkkM0e3Q0
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed c73fc5c in 1 minute and 9 seconds. Click for details.
- Reviewed
29lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:90
- Draft comment:
Ensure that updating both GITHUB_PATH and GITHUB_ENV is intentional. GITHUB_PATH (with just the directory value) automatically updates PATH for subsequent steps, while the export updates the current step. This may lead to duplicate PATH entries if not managed carefully. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .github/workflows/build-cli-release.reusable.yaml:98
- Draft comment:
For robustness, quote environment variable expansions (e.g. use "ls -la "$HOME/go/bin/"") to handle paths with spaces. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_u0Ze66i3avo84SDa
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed b350333 in 1 minute and 18 seconds. Click for details.
- Reviewed
28lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:73
- Draft comment:
Removed early 'dtolnay/rust-toolchain' step. Ensure no earlier steps depend on the Rust toolchain since its initialization has been deferred. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .github/workflows/build-cli-release.reusable.yaml:104
- Draft comment:
Added dtolnay/rust-toolchain step after the protoc test. Confirm that this later ordering properly sets up the Rust environment required for the CFFI build. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_eXNXP3AKGTG9Gowx
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 593b73c in 1 minute and 35 seconds. Click for details.
- Reviewed
31lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:88
- Draft comment:
Good: Explicitly specifying 'shell: bash' here ensures consistent shell behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_4YqC1MD2Lx15ENLD
You can customize
| which protoc-gen-go | ||
| echo "Testing protoc can find protoc-gen-go plugin..." | ||
| # Test that protoc can find the go plugin (this will show an error about missing .proto file, but that's expected) | ||
| protoc --go_out=/tmp --help | grep -q "go_out" && echo "✅ protoc recognizes --go_out flag" || echo "❌ protoc does not recognize --go_out flag" |
There was a problem hiding this comment.
Consider failing the step if '--go_out' isn’t recognized instead of only echoing a message.
Suggested change
| protoc --go_out=/tmp --help | grep -q "go_out" && echo "✅ protoc recognizes --go_out flag" || echo "❌ protoc does not recognize --go_out flag" | |
| protoc --go_out=/tmp --help | grep -q "go_out" && echo "✅ protoc recognizes --go_out flag" || (echo "❌ protoc does not recognize --go_out flag"; exit 1) |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 57954f0 in 43 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:150
- Draft comment:
Prepending $HOME/go/bin to PATH ensures that Go-installed tools (e.g., protoc-gen-go) are available during the CFFI build. Verify that this matches the setup elsewhere. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_YI7e0Xlz9f5cVZQo
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ce6558d in 1 minute and 8 seconds. Click for details.
- Reviewed
21lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:149
- Draft comment:
Good use of 'shell: bash' to enable multiline commands. Ensure consistency across similar steps. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. .github/workflows/build-cli-release.reusable.yaml:150
- Draft comment:
Setting PROTOC_ARGS to specify the go plugin path is a neat fix for cross-compilation. Confirm the plugin path remains valid if installation location changes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_RWCJ3vxKzBqSxD6N
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5621bc0 in 1 minute and 57 seconds. Click for details.
- Reviewed
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:96
- Draft comment:
Added an 'id' to capture the protoc-gen-go path output. Consider adding a check to fail early if the command returns an empty path. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The script already has robust error handling and verification. Lines 107-109 explicitly test if protoc can find and use the plugin. The suggestion to add another check seems redundant given the existing verification steps. The script will fail naturally with clear error messages if protoc-gen-go is not available. I could be underestimating the value of an explicit empty path check. Maybe there's a specific failure mode I'm not considering where the path could be empty but other checks pass. The existing checks are comprehensive - they verify the binary exists, is in PATH, and can be used by protoc. An empty path check would be redundant since 'which' will fail if the binary isn't found. The comment should be deleted as it suggests adding redundant error checking to code that already has robust verification and error handling.
2. .github/workflows/build-cli-release.reusable.yaml:159
- Draft comment:
Changed the build flag from '--release' to '--debug'. Confirm that this change is intentional, as release builds are typically expected for production artifacts. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_gcd3qtMFi00NN5c6
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 580829f in 2 minutes and 2 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_79zn2PWcNeyVL4za
You can customize
| echo "Using PROTOC_GEN_GO_PATH: $PROTOC_GEN_GO_PATH" | ||
| # We must use PROTOC_GEN_GO_PATH to explicitly tell the build script | ||
| # where to find the plugin, as the cross-compile environment is isolated. | ||
| ${{ env.CARGO }} build -p baml_cffi ${{ env.TARGET_FLAGS }} |
There was a problem hiding this comment.
Removed the invalid '--debug' flag. Note: Cargo defaults to a debug build, but later steps reference artifacts in the release directory. Confirm if a release build (i.e., using --release) is intended for consistency.
Suggested change
| ${{ env.CARGO }} build -p baml_cffi ${{ env.TARGET_FLAGS }} | |
| ${{ env.CARGO }} build --release -p baml_cffi ${{ env.TARGET_FLAGS }} |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 7d829bc in 2 minutes and 3 seconds. Click for details.
- Reviewed
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:159
- Draft comment:
Consider quoting $HOME/go for robustness (e.g., export GOPATH="$HOME/go"). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .github/workflows/build-cli-release.reusable.yaml:160
- Draft comment:
Consider quoting $GOPATH/bin (e.g., export GOBIN="$GOPATH/bin") for script safety. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. .github/workflows/build-cli-release.reusable.yaml:161
- Draft comment:
Consider using $GOROOT/bin instead of $GOROOT and remove $GOPATH from PATH unless needed (e.g., export PATH="$PATH:$GOROOT/bin:$GOBIN"). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment appears technically correct - $GOROOT/bin is more specific than $GOROOT and including full $GOPATH in PATH is usually unnecessary when you have $GOBIN. However, this is in a build script where being explicit and inclusive with paths is often preferred over being minimal. The change works as-is and the suggestion is more of an optimization than fixing a real issue. The current PATH setup, while not minimal, may be intentionally broad to ensure all necessary executables are found across different build environments. The suggested change could potentially break something. While the comment is technically accurate, the current approach is working and the suggested change is more of an optional optimization than fixing a real problem. This comment should be removed as it suggests an optional optimization rather than fixing a clear issue, and the current working approach may be intentionally broad for build reliability.
Workflow ID: wflow_VNT03kJCRyfbYiac
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed f5f88b5 in 2 minutes and 44 seconds. Click for details.
- Reviewed
82lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:154
- Draft comment:
The conditional assignment for PROTOC_GEN_GO_PATH based on env.CARGO is a good fix; consider adding a check to verify the expected file exists at that path before proceeding. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The workflow already has a comprehensive test step for protoc-gen-go availability. The "Test protoc-gen-go availability" step on line 96 already verifies the tool is available and working by running 'which protoc-gen-go' and testing the protoc command. Adding another check would be redundant. If the tool wasn't available, the build would fail naturally anyway. I could be wrong about the comprehensiveness of the existing checks - maybe they don't cover the cross-build container case fully. Even if the existing checks don't cover every case, the build would fail naturally and clearly if protoc-gen-go was missing. An extra check wouldn't add meaningful value. The comment should be deleted. The workflow already has sufficient verification of protoc-gen-go, and an additional check would be redundant without adding value.
2. engine/Cross.toml:15
- Draft comment:
The pre-build commands are largely duplicated across multiple target configurations. Refactoring to eliminate redundancy (or using shared configuration if supported) would improve maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_a46k0FpjXBM1EeWT
You can customize
| pre-build = [ | ||
| "apt-get update", | ||
| "apt-get install -y wget", | ||
| "wget -q https://go.dev/dl/go1.21.13.linux-amd64.tar.gz", |
There was a problem hiding this comment.
For the aarch64 targets, the pre-build script downloads a Go tarball for linux-amd64. Using an AMD64 binary for an ARM target may lead to toolchain issues. Consider switching to the ARM64 tarball (e.g. go1.21.13.linux-arm64.tar.gz).
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 0aa4dab in 1 minute and 39 seconds. Click for details.
- Reviewed
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/Cross.toml:38
- Draft comment:
Same as above: consider a fallback for unsupported systems and verify if yum should run an update step for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The yum update suggestion seems reasonable for consistency since apt-get and apk both do updates. The fallback message would be helpful for debugging. However, this is a build script and the environments it runs in are likely controlled/known. The current code handles the three major package managers, and a failure would be obvious without an explicit message. I might be overestimating how controlled the build environment is. Missing an update step for yum could potentially cause package conflicts or failed installations. While the suggestions would make the script more robust, they aren't critical changes. The script will either work or fail obviously in its current form. The suggestions are nice-to-have improvements but not important enough to warrant a comment, especially given that the build environments are likely controlled.
Workflow ID: wflow_PDSFJD1WyqTYUHMT
You can customize
|
|
||
| [target.x86_64-unknown-linux-musl] | ||
| pre-build = [ | ||
| "if command -v apk >/dev/null 2>&1; then apk update && apk add wget; elif command -v apt-get >/dev/null 2>&1; then apt-get update && apt-get install -y wget; elif command -v yum >/dev/null 2>&1; then yum install -y wget; fi", |
There was a problem hiding this comment.
Consider handling the case when none of the expected package managers are found. Also, note that the yum branch doesn’t update packages while others do.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d6b0df7 in 2 minutes and 2 seconds. Click for details.
- Reviewed
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/Cross.toml:27
- Draft comment:
Ensure the aarch64-unknown-linux-gnu section downloads the correct linux-arm64 tarball. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that a specific section downloads the correct tarball. This is similar to asking the author to double-check or verify something, which is against the rules. The comment does not provide a specific suggestion or point out a clear issue.
2. engine/Cross.toml:38
- Draft comment:
Correct the download URL in aarch64-unknown-linux-musl to use the arm64 archive. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_DHWDkd4iaEOJJHb1
You can customize
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| toolchain: stable | ||
| target: ${{ matrix._.target }} | ||
|
|
||
| - uses: Swatinem/rust-cache@v2 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed cbc9c3b in 1 minute and 26 seconds. Click for details.
- Reviewed
55lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:76
- Draft comment:
Good cleanup: duplicate rust-cache step removed. Please verify its new placement still ensures correct caching. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. engine/Cross.toml:8
- Draft comment:
The tarball URL changed from linux-arm64 to linux-amd64. Confirm this is correct for all targets (especially for aarch64 targets, which typically need the arm64 tarball). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_0C7dYbXBOTxcOOpb
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ddf54ba in 53 seconds. Click for details.
- Reviewed
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/build-cli-release.reusable.yaml:141
- Draft comment:
Uncommenting the CLI build step ensures the binary is built with --release, static-ssl, and no-default-features. Confirm that these options are desired for all targets. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm their intention regarding the build options. This violates the rule against asking the author to confirm their intention or to ensure the behavior is intended. Therefore, this comment should be removed.
2. .github/workflows/build-cli-release.reusable.yaml:166
- Draft comment:
Adding the '--release' flag for the CFFI build ensures consistency with the CLI build. Verify this change matches your intended release mode settings. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify their intention regarding the '--release' flag. This violates the rule against asking the author to confirm their intention. However, the first part of the comment is informative, explaining the purpose of the change. Overall, the comment is not actionable and does not provide a specific suggestion or request for a test.
Workflow ID: wflow_jRlzL8uifdisAR6J
You can customize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Fixes CFFI build by updating
protoc-gen-goinstallation and usage in build scripts and workflows..github/workflows/build-cli-release.reusable.yamlto installprotoc-gen-gointo.cargo/binand test its availability.Cross.tomlfor cross-compilation configuration, installingprotoc-gen-gofor various Linux targets.build.rsto allow overridingprotoc-gen-goplugin path usingPROTOC_GEN_GO_PATHenvironment variable.PROTOC_GEN_GO_PATHbased on build environment (native vs cross) inbuild-cli-release.reusable.yaml.protoc-gen-gois available in the PATH for both native and cross-compilation.pushtrigger forprotoc-fixbranch in.github/workflows/build-cli-release.reusable.yaml..github/workflows/build-cli-release.reusable.yamlfor clarity.This description was created by
for ddf54ba. You can customize this summary. It will automatically update as commits are pushed.