Open skraus-dev opened 1 year ago
Hi! Thanks for the report
I am not sure if we are testing dynamic linking in CI so this might be a new use case.
You can see how build script first attempts to find the dynamic libary by
Both of the approaches fail and then it reverts to vendored build.
Vendored build fails with link (as you posted). Link is failing because there was no build done. Build was not done because nmake is not found, as far as I can tell:
CMake Error at CMakeLists.txt:1 (project): [libcec-sys 4.0.3] Running [libcec-sys 4.0.3] [libcec-sys 4.0.3] 'nmake' '-?' [libcec-sys 4.0.3] [libcec-sys 4.0.3] failed with: [libcec-sys 4.0.3] [libcec-sys 4.0.3] The system cannot find the file specified
So something is off with visual studio/build setup? The vendored build is tested in CI even.
But to get it to link to the provided shared library/dll, we would like to get the smoke tests working
smoke_abi6.obj : error LNK2019: unresolved external symbol __imp_libcec_initialise referenced in function main
[libcec-sys 4.0.3] smoke_abi6.obj : error LNK2019: unresolved external symbol _Static_assert referenced in function main
This is failure in linking phase so it seems libcec header is found but dll not. Have you found Microsoft documentation on the env variable to set? In linux this is controlled by LD_LIBRARY_PATH.
It also seems that static assertion is not supported by your compiler? We can diagnose this after fixing the libcec linking error -- build script might need some fixing. Found good reference https://learn.microsoft.com/en-us/cpp/c-language/static-assert-c?view=msvc-170
Seems like PATH should influence the dll finding process on windows, see https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order
Does that particular error disappear with PATH set to C:\Program Files (x86)\Pulse-Eight\USB-CEC Adapter\x64 ?
Pinging @ok-nick from https://github.com/ssalonen/libcec-sys/pull/15 (introduction of windows support)
I noticed the following syntax from Microsoft documentation https://learn.microsoft.com/en-us/cpp/build/reference/compiler-command-line-syntax?view=msvc-170
We are not passing library names to link to now in smoke test build, should we?
Seems like PATH should influence the dll finding process on windows, see https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order Does that particular error disappear with
PATH
set toC:\Program Files (x86)\Pulse-Eight\USB-CEC Adapter\x64
?
Nope, does not solve it unfortunately - it still fails the smoke test. I also attempted to modify the build.rs
script and appended /link cec
or /link cec.dll
- With /link cec.dll
it actually complained about not finding cec.dll
.
Both of the approaches fail and then it reverts to vendored build.
Correct me if I am wrong, but this feels a bit unintuitive. I understand the idea of best effort when compiling libcec-sys, but it should only do what it's been told - and if not fullfillable - fail hard.
I am continuing research and ultimately targeting to help with simplifying the burden on the user (installing the rather large VS suite.. and a specific build of python etc.) by providing a CI-built repository that includes the different prebuilt flavors of libcec for Windows.
The repository or let's rather say the github workflow is 75% done now finished.
Result: https://github.com/skraus-dev/libcec-vendor
What's missing still is the commit of the binary files back into the repository, so the repo can be easily included as a submodule to consume the bins from there.
PS: If this goes too much off-topic, happy to continue in https://github.com/ssalonen/libcec-sys/issues/24.
PPS: Why use prebuilts? I took the idea from https://github.com/ftdi-rs/libftd2xx-ffi/blob/main/build.rs which just has a quite minimalistic build.rs that does not attempt to build libs and rather just emits compiler directives (except for bindgen). Tbf, libftd2xx does not have the option to compile from source, as the manufacturer FTDI only provides binaries.
Quick comments from my phone
Re, vendored feature etc: I have followed the model from some of the major sys crates, should not be too surprising. I see the default feature set as "best effort" not really as "non-vendored"
Re. Prebuilt libraries: I do not completely follow, would not the official libcec distribution included the needed binaries? Or are referring to needed libraries / artifacts for static linking?
Re /link, good idea experimenting correct flags. Is it always complaining not finding the right libcec symbol?
Do you agree that we should get that standalone C program build working first? This should be basic C/MSVC stuff, not related to rust at this level. Unfortunately, my understanding is limited
Re. Prebuilt libraries: I do not completely follow, would not the official libcec distribution included the needed binaries? Or are referring to needed libraries / artifacts for static linking?
I am referring to both, dynamic and static linking.
This sums is up: https://learn.microsoft.com/en-us/cpp/build/linking-an-executable-to-a-dll?view=msvc-170
From my understanding, MSCV relies on a *.lib to link against either way, statically or dynamically.
In libcec's case:
You would use the DLL in cases when defining
DLLImport
in c# code, for marshalling the DLL at runtime.LoadLibrary
and resolving functions via GetProcAddress
Bottom-line: Pulse-Eight's Libcec distribution for windows ONLY contains the DLL.
Do you agree that we should get that standalone C program build working first?
Sure!
Is it always complaining not finding the right libcec symbol?
It's solved now:
-> Manual build of libcec via cmdline in Developer Command Prompt worked from this point on.
libcec_sys (vendored) is compiled also successfully, when started from "Developer command prompt".
Ensure the theory about *.lib files by experimenting with the linker directives in build.rs
Hey, I switched to Linux a while ago so I'm not sure if I could be much help. If you have any other questions I will do my best to answer.
@skraus-dev good info, today I learned sometving.
It looks like it might be possible to generate .lib from .dll? https://stackoverflow.com/questions/9946322/how-to-generate-an-import-library-lib-file-from-a-dll
Also, if there's a lot of changes, I propose to rebase on top ci-speedup branch, there are some heavier github CI and build.rs changes. I need to clean up the branch a bit still and merge to main (EDIT: now merged)
Thanks @ok-nick , seems like we figured out the necessary steps
Once again regarding preference of vendored vs non-vendored, not sure where I picked that up. Some examples
I do see similar type of logic is used for static vs dynamic building in openssl-sys: https://github.com/sfackler/rust-openssl/blob/master/openssl-sys/build/main.rs#L351-L398 (if static is explicitly requested, we do static build. Otherwise try to do dynamic build).
However, same crate uses vendored sources only if vendored
feature is enabled.
https://github.com/rust-lang/git2-rs/blob/master/libgit2-sys/build.rs
Uses vendored when explicitly requested. Vendored can be also disabled with an env variable (LIBGIT2_NO_VENDOR)
// Specify `LIBGIT2_NO_VENDOR` to force to use system libgit2.
// Due to the additive nature of Cargo features, if some crate in the
// dependency graph activates `vendored` feature, there is no way to revert
// it back. This env var serves as a workaround for this purpose.
Even if user has not explicitly specified the vendored
feature, libgit2-sys
seems to fall back to vendored build when linking to system lib has failed.
https://github.com/alexcrichton/bzip2-rs/blob/master/bzip2-sys/build.rs
With static
: always use vendored
Without static
: prefer system lib, but allow fallback to vendored
So yeah, as summary, practices seem to vary between popular crates...
Ideally it would be nice to have the following
Thoughts?
- find out the visual studio installation and enable it automatically (similar to CI and batch scripts in libcec source) - no need for "developer command prompt"
Yeah that sounds good, to not rely on Developer command prompt environment but rather source the environment ourselves, if found.
this step should then also verify the existance of components cmake
(as mentioned in your last bullet-point), nmake
and python
- detect libcec installation in the default folder, and link to it statically by default. Hopefully we could export ".lib" from the ".dll"
While it's possible to forge a *.lib
from a DLL, this would still be a dynamic linking - as by default, Pulse-Eight does not deliver the cec-static.lib
import library..
Concerning DLL -> Import Library, the new rust 1.71.0 release introduced a nice feature, but that's on you to decide if it's feasible to implement. Allows defining linkage to a DLL inline in code.
PR: https://github.com/rust-lang/rust/pull/109677/
RFC: https://rust-lang.github.io/rfcs/2627-raw-dylib-kind.html
- provide static feature (windows only), and if enabled, compile the binary against (vendored) libcec statically. I believe currently static build is always part of the vendored build on windows (see Support static linking on Windows #24 (comment) )
Introducing a distinct handling for static linking sounds good!
Static linking the C runtime looks independent from the userspace-library linking.
See: https://doc.rust-lang.org/reference/linkage.html#static-and-dynamic-c-runtimes
- with vendored build, error out if cmake is missing
See answer to point 1
also verify the existance of components cmake (as mentioned in your last bullet-point), nmake and python
👍 minor correction: python should not be needed anymore: https://github.com/ssalonen/libcec-sys/blob/07859ab7676e79fbe196040f950cce54e2945af1/build/build.rs#L109
While it's possible to forge a *.lib from a DLL, this would still be a dynamic linking - as by default, Pulse-Eight does not deliver the cec-static.lib import library..
I see, got it now.
Concerning DLL -> Import Library, the new rust 1.71.0 release introduced a nice feature, but that's on you to decide if it's feasible to implement. Allows defining linkage to a DLL inline in code.
This sounds something worth investigation!
1.71.0 is quite new though...
Static linking the C runtime looks independent from the userspace-library linking.
Yes, "static" feature would only control libcec link, not the C runtime.
Bug description
Attempting to build a project which depends on
libcec-sys
orcec-rs
yields error:LINK : fatal error LNK1181: cannot open input file 'cec.lib'
Smoke tests apparently all fail...
Will be happy to send a PR for more detailed README in that regard, if it can be narrowed down.
To Reproduce
Cargo.toml
main.rs
C:\Program Files (x86)\Pulse-Eight\USB-CEC Adapter\x64\cec.dll
C:\Program Files (x86)\Pulse-Eight\USB-CEC Adapter\x64
as that made a bit more sense to me, still, same result.C:\Program Files (x86)\Pulse-Eight\USB-CEC Adapter\include
Developer command prompt
or regularcmd
cargo build
orcargo build --release
Expected behavior
Build process of libcec-sys recognizing the
cec.dll
and dynamically linking with it.Environment
Additional context
Verbose output for
cargo build -vv