Conversation
slang-sys/Cargo.toml
Outdated
| bindgen = "0.72.0" | ||
|
|
||
| [features] | ||
| default = ["static"] |
There was a problem hiding this comment.
I'm open to make dynamic the default
There was a problem hiding this comment.
I'd prefer dynamic be the default as long as Slang doesn't distribute static libraries in their releases or through the Vulkan SDK.
ThierryBerger
force-pushed
the
static-build
branch
from
a080022 to
d58c520
Compare
slang-sys/build.rs
Outdated
| println!("cargo:rustc-link-lib=static=miniz"); | ||
| println!("cargo:rustc-link-lib=static=lz4"); | ||
| // Standard C++ library | ||
| println!("cargo:rustc-link-lib=static=stdc++"); |
There was a problem hiding this comment.
it may be better to link dynamically for stdc++ ? This would remove the added stdc++ path env variable, and ends up being easier to use on apple:
println!("cargo:rustc-flags=-l dylib=c++");But this doesn't seem to work on my popOS ; I'm not sure if I should expose somehow my libc++ or conditionally choose the strategy depending on OS.
There was a problem hiding this comment.
https://github.com/dtolnay/link-cplusplus is probably a better choice
There was a problem hiding this comment.
I'm mostly familiar with Windows, so not sure of the requirements or best solutions for Apple/Linux.
If we could do this without additional dependencies, however small, that would be great.
There was a problem hiding this comment.
continued discussion on https://github.com/FloatyMonkey/slang-rs/pull/25/files#r2410607835
laurooyen
left a comment
laurooyen
left a comment
There was a problem hiding this comment.
Thanks for this PR, even updating the documentation!
Static linking is certainly a welcome addition, just a shame Slang doesn't distribute static libraries by default.
slang-sys/build.rs
Outdated
| println!("cargo:rustc-link-lib=static=miniz"); | ||
| println!("cargo:rustc-link-lib=static=lz4"); | ||
| // Standard C++ library | ||
| println!("cargo:rustc-link-lib=static=stdc++"); |
There was a problem hiding this comment.
I'm mostly familiar with Windows, so not sure of the requirements or best solutions for Apple/Linux.
If we could do this without additional dependencies, however small, that would be great.
slang-sys/Cargo.toml
Outdated
| bindgen = "0.72.0" | ||
|
|
||
| [features] | ||
| default = ["static"] |
There was a problem hiding this comment.
I'd prefer dynamic be the default as long as Slang doesn't distribute static libraries in their releases or through the Vulkan SDK.
…ng ; fix apple build.rs
… both my mac and linux in this state.
| if cfg!(not(target_env = "msvc")) { | ||
| // MinGW: Link libstdc++ | ||
| println!("cargo:rustc-link-lib=stdc++"); | ||
| } else { | ||
| // No linking necessary on Windows, the MSVC runtime is linked by default. | ||
| } |
There was a problem hiding this comment.
Those paths haven´t been tested, so please report your findings :)
| // The C++ lib has to be the same as the one used to compile Slang, | ||
| // otherwise you will get errors about missing symbols. | ||
| // The following is only a best guess depending on platform's defaults. |
There was a problem hiding this comment.
I added this note as I was reading through https://github.com/dtolnay/link-cplusplus/blob/master/README.md and other resources more carefully.
I believe link-cplusplus solution is better, as in this PR, a user on linux could build the slang lib against libc++ (not stdc++), and then shader-slang-sys wouldn't find c++ symbols due to incompatible ABIs...
As an actionable call for action, I recommend either:
- keep it as-is, best effort is better than no effort, adjust when/if complaints arise.
- add
link-cplusplusdependency - vendor/copy
link-cpluspluscode in slang-rs.
Your call :)
|
@laurooyen I addressed the feedbacks, and tested successfully on mac + linux (popOS).
|
2 participants
Hello, linking dynamically requires library users to either:
It also makes it difficult to have a self-contained binary, where you just move it anywhere and it works.
An alternative is to statically link as much as we can. Users still need to provide path to libraries when compiling, but the above requirements can be ignored.
This PR adds a feature gate to statically link slang, this linking requires an additional
SLANG_EXTERNAL_DIRenv variable.I built my slang library from source, setting
"SLANG_LIB_TYPE": "STATIC"in theCMakePresets.json.