search

ssalonen / libcec-sys

FFI bindings for the libcec
GNU General Public License v2.0
2 stars 3 forks source link

Windows support #15

Closed ok-nick closed 2 years ago

ok-nick commented 2 years ago

Closes #13

Support for Windows is fairly straightforward and libcec takes care of most of the work via their batch files.

Prerequisites:

TODO

For now, I've hardcoded a few parameters just for testing.

ok-nick commented 2 years ago

I keep getting the error build/smoke_abi6.c(2): fatal error C1083: Cannot open include file: 'cecc.h': No such file or directory for each ABI version when running the smoke test. I must be linking the wrong directory, but I'm not sure which.

This is what the LIBCEC_BUILD/amd64 directory looks like, and is what rustc-link-search is being set to.

│   cec-client.exe
│   cec-static.lib
│   cec.dll
│   cec.lib
│   cecc-client.exe
│
├───include
│   ├───libcec
│   │       cec.h
│   │       cecc.h
│   │       ceccloader.h
│   │       cecloader.h
│   │       cectypes.h
│   │       version.h
│   │
│   └───p8-platform
│       │   os.h
│       │
│       ├───sockets
│       │       cdevsocket.h
│       │       socket.h
│       │       tcp.h
│       │
│       ├───threads
│       │       atomics.h
│       │       mutex.h
│       │       threads.h
│       │
│       ├───util
│       │       atomic.h
│       │       buffer.h
│       │       StdString.h
│       │       StringUtils.h
│       │       timeutils.h
│       │       util.h
│       │
│       └───windows
│               dlfcn-win32.h
│               os-socket.h
│               os-threads.h
│               os-types.h
│
├───lib
│   │   cec-static.pdb
│   │   cec.pdb
│   │   p8-platform.lib
│   │   p8-platform.pdb
│   │
│   └───p8-platform
│           p8-platform-config.cmake
│
└───python
        pyCecClient.py

Any ideas @ssalonen?

ssalonen commented 2 years ago

The idea of smoke test is to compile a small c application, including headers from cec project and linking to libcec shared library.

Note that intent is not utilize vendor library in any way here.

Even on linux this does not work by default, as the directories are not findable in the common distros without extra flags/environmental variables.

However, when setting those environment variables, it offers the user a viable option without pkg config (or without pkg config files). You can find concrete example from github CI flow where this is tested.

Are there similar same environment variables on windows? Is there precompiled distribution of libcec for windows?

ssalonen commented 2 years ago

Based on quick googling visual studio might utilize these directories and env variables

If this is correct, it makes sense to keep the same smoke test with windows as well.

Is this something you can try out at some point? I am not sure how hard would it be to setup windows github action to test all this... There seems to be supporting actions available https://github.com/marketplace/actions/setup-msbuild

ok-nick commented 2 years ago

Yes, that sounds about right. The build script runs every compilation as to why I thought the smoke tests would fix it. There must be something in the libcec build script causing a file to be modified at the last second.

I'll have to check out the github actions once everything looks good.

ok-nick commented 2 years ago

I added a basic Windows test for vendored builds and it works well. The reason the --release flag was added to cargo test is because a debug build requires Python with debug binaries installed, which is not something github actions have by default.

I'm not sure what cross does or the point of using it in your situation, so I'm not sure if it is needed for CI on Windows?

ssalonen commented 2 years ago

I understand the issue without the --release. Ideally it would make sense not use --release as some of the rust assertions are not active then.

On the other hand, there are no tests at all in libcec-sys, so it's not a big deal here, more of a topic for cec-rs.

Can we not compile the vendored build always as non-debug? I am not sure but is this how it goes with nix builds?

Cross is not needed in windows here. In fact, it does not seem to support msvc, only gnu compiler https://github.com/cross-rs/cross

Cross is used for cross compiling and running the tests with different architectures. My thinking was that this library might be used with different type of embedded environments / devices, so why not introduce the compilation check for it as well. However, since there is little platform specific code, it is not doing much now...

ssalonen commented 2 years ago

BTW, on windows, does this build the library as statically linked to the application binary?

Should it? I am not sure which way is the preferred way on windows.

ok-nick commented 2 years ago

We could technically always compile libcec in release mode, although I'm assuming it may cancel out some of the debug assertions and other nice features. I guess the same also goes for Rust, so I'll add the correct python installation to CI.

As for Cross, if it is no longer useful, do you think we should remove it?

By default, according to here, it will compile dynamically before statically. I don't see any use for a dynamic library, especially when everything Rust is pretty much compiled statically and upstream crates will probably expect this to just "work," so I'll add an explicit check.

ok-nick commented 2 years ago

I keep running into issues statically linking. When linking against cec.lib, it ends up linking back to cec.dll. When linking against cec-static.lib, it explodes with linking errors.

ok-nick commented 2 years ago

There is also no easy way to find installed Visual Studio versions. cc seems capable of this, but it only exposes a method for getting the latest installed version (if they have 2022 then we can't know if they have 2019), check here.

An alternative would be to use the vswhere utility, which comes preinstalled with Visual Studio 2017+. Since libcec theoretically supports down to 2013, we would have to manually download the executable. In addition, we would have to pull in this dependency, which unfortunately hasn't been maintained in over 4 years. I'm not sure if there is anything else.

Edit: Although libcec seems to compile fine with 2017, the docs state it only supports 2019+ so there could possibly be side effects. We should probably only support 2019 and if it's not installed, libcec will automatically output a corresponding error.

ssalonen commented 2 years ago

When testing static linking, did you remove the cargo:rustc-link-lib=cec? I think it is the thing that adds the link?

EDIT: According to rustc docs, pointed by build script docs

If the kind is not specified in the link attribute or on the command-line, it will link a dynamic library if available, otherwise it will use a static library.

They talk only about the attribute and cli arg but you would imagine the same applies for build scripts...

ok-nick commented 2 years ago

No, I kept it as is and even tried changing it to cec-static as well as adding the explicit static kind as described in the docs.

ok-nick commented 2 years ago

You may be able to see for yourself if you fork my branch and run the CI. Tomorrow... well, later today for me, I could send u the output I get.

There isn't much problem linking against a dynamic lib, but then it would have to be packaged with the binary I believe? It would definitely be preferable to statically link.

ssalonen commented 2 years ago

Yes, I would expect static linking with windows while keeping dynamic linking with Linux. With Linux libcec is often easily available from distribution repos.

We can always make this configurable as well, if need arises in the future.

ssalonen commented 2 years ago

There is also no easy way to find installed Visual Studio versions. cc seems capable of this, but it only exposes a method for getting the latest installed version (if they have 2022 then we can't know if they have 2019), check here.

An alternative would be to use the vswhere utility, which comes preinstalled with Visual Studio 2017+. Since libcec theoretically supports down to 2013, we would have to manually download the executable. In addition, we would have to pull in this dependency, which unfortunately hasn't been maintained in over 4 years. I'm not sure if there is anything else.

Edit: Although libcec seems to compile fine with 2017, the docs state it only supports 2019+ so there could possibly be side effects. We should probably only support 2019 and if it's not installed, libcec will automatically output a corresponding error.

Was this resolved? Why is not ok to utilize the latest installed version?

ssalonen commented 2 years ago

Regarding cross:

On second thought it makes sense to ensure the build works, e.g. with arm (raspberry pi). This way we would get early failure already with libcec-sys, not with cec-rs, in case the build needs some adjustment.

With windows we are only looking at non-gnu compilation (not mingw), thus, cross is not usable there.

ok-nick commented 2 years ago

libcec does not compile with Visual Studio 2022. It does compile with 2017-2019 (as far as I've tested) although the docs state it only supports 2019+. I will make it so you must have 2019 installed.

ok-nick commented 2 years ago

Should definitely cache python, it takes 1-2 minutes to install.

The only thing left is to support static linking and add to the markdown files. I'll probably leave static linking for another PR, seems like it will be a pain.

ssalonen commented 2 years ago

Thank you! This has been quite some work but really great to see windows supported as well 🙏

ssalonen commented 2 years ago

Big thank you!