add tracing_subscriber needed rust flags for each binding #1143
Conversation
|
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.
❌ Changes requested. Reviewed everything up to ad3fdf3 in 54 seconds
More details
- Looked at
126lines of code in6files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_gov0JdkHaeeiwkrv
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -0,0 +1,9 @@ | |||
| # to enable json logging we need this serialization feaure. https://docs.rs/tracing-subscriber/latest/tracing_subscriber/#unstable-features | |||
There was a problem hiding this comment.
Typo in comment: 'feaures' should be 'features'. This typo is also present in engine/language_client_ruby/.cargo/config.toml and engine/language_client_typescript/.cargo/config.toml.
| # to enable json logging we need this serialization feaure. https://docs.rs/tracing-subscriber/latest/tracing_subscriber/#unstable-features | |
| # to enable json logging we need this serialization feature. https://docs.rs/tracing-subscriber/latest/tracing_subscriber/#unstable-features |
| @@ -0,0 +1,9 @@ | |||
| # to enable json logging we need this serialization feaure. https://docs.rs/tracing-subscriber/latest/tracing_subscriber/#unstable-features | |||
| [build] | |||
There was a problem hiding this comment.
The rustflags setting in language_client_ruby is identical to those in language_client_python and language_client_typescript. Consider consolidating these configurations to avoid duplication.
- Cargo config for language_client_python (config.toml)
- Cargo config for language_client_typescript (config.toml)
| @@ -0,0 +1,4 @@ | |||
| # to enable json logging we need this serialization feaure. https://docs.rs/tracing-subscriber/latest/tracing_subscriber/#unstable-features | |||
| [build] | |||
There was a problem hiding this comment.
This Cargo configuration file is a duplicate of the existing 'engine/.cargo/config.toml'. Consider reusing the existing configuration instead of adding a new one.
- engine/.cargo/config.toml (config.toml)
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 54be423 in 9 seconds
More details
- Looked at
29lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_12zJUAavcWX7Vfpy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 88976f1 in 20 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:98
- Draft comment:
The change toshared-key: engine-${{ github.job }}-${{ matrix._.target }}-v2is a good practice for versioning and avoiding cache conflicts. - Reason this comment was not posted:
Confidence changes required:0%
The change in the shared-key value fromengine-${{ github.job }}-${{ matrix._.target }}toengine-${{ github.job }}-${{ matrix._.target }}-v2is a versioning update. This is likely intended to avoid cache conflicts with previous versions, which is a good practice.
Workflow ID: wflow_1Mr7xaqVixT5D0h7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9e8f729 in 12 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:86
- Draft comment:
Disabling the cache (cache: false) might lead to longer build times. If this is intentional, consider documenting the reason for future reference. - Reason this comment was not posted:
Confidence changes required:50%
The cache is disabled, which might lead to longer build times. If this is intentional, it should be documented.
Workflow ID: wflow_lbR7LDQU1xvdGauf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 94ceeb7 in 11 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:86
- Draft comment:
Consider enabling pnpm cache to improve build performance. Caching can significantly reduce build times by reusing previously downloaded dependencies. - Reason this comment was not posted:
Confidence changes required:50%
The cache setting for pnpm is commented out, which might lead to slower builds due to lack of caching. This is a potential performance issue.
Workflow ID: wflow_lYqj404FnrsdeEwo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 30c01d3 in 16 seconds
More details
- Looked at
59lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/build-typescript-release.reusable.yaml:60
- Draft comment:
RUSTFLAGS are set multiple times in this file (lines 60 and 71). Consider consolidating them to avoid potential inconsistencies. - Reason this comment was not posted:
Confidence changes required:50%
The RUSTFLAGS are being set multiple times in the same workflow file, which can lead to confusion and potential errors if they are not consistent. It's better to consolidate them into a single definition if possible.
2. .github/workflows/build-typescript-release.reusable.yaml:71
- Draft comment:
RUSTFLAGS are set multiple times in this file (lines 60 and 71). Consider consolidating them to avoid potential inconsistencies. - Reason this comment was not posted:
Confidence changes required:50%
The RUSTFLAGS are being set multiple times in the same workflow file, which can lead to confusion and potential errors if they are not consistent. It's better to consolidate them into a single definition if possible.
Workflow ID: wflow_BL8AeIeXvutrah8j
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 204ffa1 in 30 seconds
More details
- Looked at
27lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/build-ruby-release.reusable.yaml:78
- Draft comment:
TheRUSTFLAGSenvironment variable should be quoted to prevent issues with spaces or special characters in the value.
RUSTFLAGS="--cfg tracing_unstable"
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_OSF2Rl8ODBxWYB15
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -57,6 +57,7 @@ jobs: | |||
| CC_x86_64_unknown_linux_musl=x86_64-linux-musl-gcc | |||
| CXX_x86_64_unknown_linux_musl=x86_64-linux-musl-g++ | |||
| AR_x86_64_unknown_linux_musl=x86_64-linux-musl-ar | |||
| RUSTFLAGS=-C target-feature=-crt-static --cfg tracing_unstable | |||
There was a problem hiding this comment.
The RUSTFLAGS environment variable should be quoted to prevent issues with spaces or special characters in the value.
| RUSTFLAGS=-C target-feature=-crt-static --cfg tracing_unstable | |
| RUSTFLAGS="-C target-feature=-crt-static --cfg tracing_unstable" |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 6d365dd in 23 seconds
More details
- Looked at
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_iPARNb4WyD0uRATv
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -66,6 +67,9 @@ jobs: | |||
| curl -LO https://musl.cc/aarch64-linux-musl-cross.tgz | |||
| tar -xzf aarch64-linux-musl-cross.tgz | |||
| echo "$PWD/aarch64-linux-musl-cross/bin" >> $GITHUB_PATH | |||
| cat >>$GITHUB_ENV <<EOF | |||
| RUSTFLAGS=-C target-feature=-crt-static --cfg tracing_unstable | |||
There was a problem hiding this comment.
The RUSTFLAGS assignment should be quoted to ensure it is treated as a single string.
| RUSTFLAGS=-C target-feature=-crt-static --cfg tracing_unstable | |
| RUSTFLAGS="-C target-feature=-crt-static --cfg tracing_unstable" |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c72c2f6 in 17 seconds
More details
- Looked at
40lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_Fxx29bUdDUKLGdg9
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Add Rust flags for
tracing_subscriberand update GitHub workflows to includeaaron-fixbranch for Python, Ruby, and TypeScript clients.rustflagsfortracing_unstablein.cargo/config.tomlfor Python, Ruby, and TypeScript clients.aaron-fixbranch to trigger builds inbuild-python-release.reusable.yaml,build-ruby-release.reusable.yaml, andbuild-typescript-release.reusable.yaml.RUSTFLAGSinbuild-ruby-release.reusable.yamlandbuild-typescript-release.reusable.yamlfor musl targets.extra_rustflagsfortracing_unstableinextconf.rb.This description was created by
for c72c2f6. It will automatically update as commits are pushed.