Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 683195b6b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Cargo.toml
Outdated
| highs_release = [] | ||
| ninja = [] | ||
| libz = [] | ||
| crt-static = [] |
There was a problem hiding this comment.
crt-static as a standalone Cargo feature
Enabling this feature only flips CMake to MultiThreaded; it never enables Rust's own +crt-static target feature. The new CI step has to add RUSTFLAGS=-C target-feature=+crt-static separately in .github/workflows/rust.yml:40-44, so downstream users doing the obvious cargo build --features crt-static still compile Rust against the dynamic MSVC runtime while HiGHS is built against the static one. On Windows MSVC that commonly becomes a runtime-library mismatch at link time, so the feature as added here does not produce a working static-CRT build by itself.
Useful? React with 👍 / 👎.
| "CMAKE_MSVC_RUNTIME_LIBRARY", | ||
| if cfg!(feature = "crt-static") { | ||
| "MultiThreaded" | ||
| } else { | ||
| "MultiThreadedDLL" |
There was a problem hiding this comment.
crt-static when discover bypasses the bundled build
This flag is only consumed inside build() here, but main() still only treats highs_release, libz, and ninja as build-only and it calls discover() before build(). In configurations like --no-default-features --features discover,crt-static (or adding discover on top of defaults), the new feature is silently ignored and callers link whatever CRT their pkg-config HiGHS was built with, even though they explicitly requested a static-CRT build. The other build-only knobs already hard-fail in this situation, so crt-static needs the same protection.
Useful? React with 👍 / 👎.
|
@Dirreke what do you think of this ? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ecbf911ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| } | ||
|
|
||
| if cfg!(feature = "discover") && crt_static { |
There was a problem hiding this comment.
This guard now panics for every discover + crt-static build, but crt-static is also used on non-MSVC targets (for example static Linux/musl builds) where the MSVC runtime mismatch described here does not apply. Because the panic runs before discover()/build(), it also blocks valid discover+build fallback configurations that could still safely build the bundled HiGHS library. Restricting this check to MSVC contexts (or to cases where discovery is actually used) would avoid breaking those legitimate builds.
Useful? React with 👍 / 👎.
|
Lgtm. |
| panic!( | ||
| "You have enabled Rust's 'crt-static' target feature, but also enabled the 'discover' feature. | ||
| \ | ||
| Discovering a system-installed HiGHS bypasses the bundled build, so highs-sys cannot ensure that HiGHS uses the same MSVC runtime. Please disable 'discover' when using '-C target-feature=+crt-static'." | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think using warning would make more sense here. We do not know which runtime configuration users are using, and if the build passes successfully, that should be fine.
2 participants
crt-staticflagalternative to #45