-
-
Couldn't load subscription status.
- Fork 0
feat: delegate vendoring of OpenSSL to git2 dependency tree #200
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
Conversation
WalkthroughConsolidated vendored OpenSSL support via git2 and removed runtime SSL certificate probing by deleting the probe helper and its invocation from startup initialization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp-linter/Cargo.toml (1)
40-40: Clarify the purpose of the separategit2dev-dependency.A
git2dev-dependency is added with thehttpsfeature (line 40), whilegit2is already a main dependency at line 23 (without features specified). This separation is unclear—either:
- If
httpsis needed for normal library operations, add it to the main dependency instead (possibly gated behind a feature likeopenssl-vendored).- If
httpsis only for testing/integration tests, document why the dev-dependency is separate from the main dependency, or consolidate them via feature declarations.Consider consolidating the git2 configuration to avoid duplication and clarify feature dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
cpp-linter/Cargo.toml(1 hunks)cpp-linter/src/run.rs(0 hunks)
💤 Files with no reviewable changes (1)
- cpp-linter/src/run.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build-rustdoc
- GitHub Check: cargo-tools
- GitHub Check: x86_64-apple-darwin
- GitHub Check: aarch64-pc-windows-msvc
- GitHub Check: arm-unknown-linux-gnueabihf
- GitHub Check: Build aarch64-pc-windows-msvc
- GitHub Check: Build x86_64-unknown-linux-musl
- GitHub Check: Build armv7-unknown-linux-gnueabihf
- GitHub Check: Build aarch64-unknown-linux-musl
- GitHub Check: Build aarch64-unknown-linux-gnu
- GitHub Check: Build x86_64-pc-windows-msvc
- GitHub Check: Build i686-pc-windows-msvc
- GitHub Check: Build x86_64-apple-darwin
- GitHub Check: Build FreeBSD
- GitHub Check: build-py-binding
- GitHub Check: Build current binary
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (windows-latest)
🔇 Additional comments (1)
cpp-linter/Cargo.toml (1)
40-40: Verify that theopenssl-vendoredfeature enables both vendored OpenSSL and HTTPS support.The
openssl-vendoredfeature currently enables onlygit2/vendored-openssl, but if users enable this feature to support HTTPS operations with vendored OpenSSL, thegit2/httpsfeature may also be necessary. Without it, HTTPS operations could fail at runtime.Consider whether the feature should be:
-openssl-vendored = ["git2/vendored-openssl"] +openssl-vendored = ["git2/vendored-openssl", "git2/https"]Or, if HTTPS is not part of the core library (only dev/test), document this intent clearly.
Can you clarify whether HTTPS git operations are used in the main library code, or if HTTPS is only needed for testing? This will determine whether the feature definition is correct.
Also applies to: 43-43
libgit2 is the only reason we rely on OpenSSL instead of using rust-native TLS solutions.
cc10c5c to
c0a6ac7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
==========================================
- Coverage 96.87% 96.87% -0.01%
==========================================
Files 14 14
Lines 3134 3131 -3
==========================================
- Hits 3036 3033 -3
Misses 98 98 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
libgit2 is the only reason we rely on OpenSSL instead of using rust-native TLS solutions.
Summary by CodeRabbit
Refactor
Chores