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

DRAFT for static linking #66

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

DRAFT for static linking #66

wants to merge 12 commits into from

Conversation

Vaiz
Copy link
Collaborator

@Vaiz Vaiz commented Jan 23, 2025

No description provided.

@Vaiz Vaiz requested a review from Copilot January 23, 2025 11:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 11 changed files in this pull request and generated 3 comments.

Files not reviewed (5)
  • symcrypt-sys/inc/buildInfo.h: Language not supported
  • symcrypt-bindgen/src/main.rs: Evaluated as low risk
  • symcrypt-sys/src/bindings/aarch64_pc_windows_msvc.rs: Evaluated as low risk
  • symcrypt-sys/src/bindings/x86_64_pc_windows_msvc.rs: Evaluated as low risk
  • .github/workflows/builld.yml: Evaluated as low risk
Comments suppressed due to low confidence (3)

symcrypt-sys/build/jitterentropy.rs:59

  • The function gcc_version_ge_490 does not handle cases where the version string might not be in the expected format, which could lead to a panic. Add a check to ensure the version string is in the expected format before attempting to parse it.
fn gcc_version_ge_490() -> bool {

symcrypt-sys/build/triple.rs:20

  • The panic message is unclear. Consider providing a more informative error message, such as 'Unsupported target configuration: OS: {target_os}, Arch: {target_arch}'.
panic!("unsupported target. OS: {target_os}, Arch: {target_arch}")

symcrypt-sys/build/main.rs:58

  • The comment should use 'long-term' instead of 'long term'.
// Note: This process is a band-aid. Long-term, our long term solution is to package manage SymCrypt for a subset of

symcrypt-sys/build/static_link.rs Show resolved Hide resolved
symcrypt-sys/build/static_link.rs Outdated Show resolved Hide resolved
symcrypt-sys/build/main.rs Outdated Show resolved Hide resolved
@Vaiz Vaiz requested a review from Copilot January 23, 2025 12:07
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (6)
  • symcrypt-sys/inc/buildInfo.h: Language not supported
  • symcrypt-sys/src/bindings/aarch64_pc_windows_msvc.rs: Evaluated as low risk
  • symcrypt-bindgen/src/main.rs: Evaluated as low risk
  • symcrypt-sys/src/bindings/x86_64_pc_windows_msvc.rs: Evaluated as low risk
  • .github/workflows/bindgen.yml: Evaluated as low risk
  • .github/workflows/builld.yml: Evaluated as low risk
Comments suppressed due to low confidence (4)

symcrypt-sys/build/static_link.rs:19

  • The function should propagate errors from compile_symcrypt_static using the ? operator.
compile_symcrypt_static(LIB_NAME, &options)?;

symcrypt-sys/build/static_link.rs:43

  • [nitpick] The symcrypt_use_asm field is set to false and never changed, making the use_asm function redundant.
symcrypt_use_asm: false,

symcrypt-sys/build/triple.rs:20

  • The error message for unsupported targets could be more descriptive. Consider changing it to: panic!("Unsupported target. OS: {target_os}, Arch: {target_arch}. Please check the supported target triples.")
panic!("unsupported target. OS: {target_os}, Arch: {target_arch}")

symcrypt-sys/build/main.rs:25

  • [nitpick] The comment could be clearer. Suggested rephrase: 'Searching the Windows/System32 path for the .lib file, which is the default location for a Windows-shipped symcrypt.dll.'
// Look for the .lib file during link time. We are searching the Windows/System32 path which is set as a current default to match the long term placement of a Windows shipped symcrypt.dll

symcrypt-sys/build/static_link.rs Show resolved Hide resolved
@Vaiz Vaiz linked an issue Jan 23, 2025 that may be closed by this pull request
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.

Add an option to vendor the build
1 participant