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

Windows support #15

Merged
merged 21 commits into from
Aug 6, 2022
Merged

Windows support #15

merged 21 commits into from
Aug 6, 2022

Conversation

ok-nick
Copy link
Contributor

@ok-nick ok-nick commented Jul 11, 2022

Closes #13

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

Prerequisites:

  • Visual Studio 2019 w/ Desktop Development with C++ and Universal Windows Platform development
  • CMake 3.12+
  • Python 3.6+ with Debug Binaries (not sure the exact min version, took it from docs)
  • and possibly more

TODO

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

@ok-nick
Copy link
Contributor Author

ok-nick commented Jul 11, 2022

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
Copy link
Owner

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
Copy link
Owner

ssalonen commented Jul 12, 2022

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
Copy link
Contributor Author

ok-nick commented Jul 17, 2022

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
Copy link
Contributor Author

ok-nick commented Jul 25, 2022

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
Copy link
Owner

ssalonen commented Jul 26, 2022

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
Copy link
Owner

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
Copy link
Contributor Author

ok-nick commented Jul 26, 2022

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
Copy link
Contributor Author

ok-nick commented Jul 26, 2022

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
Copy link
Contributor Author

ok-nick commented Jul 27, 2022

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
Copy link
Owner

ssalonen commented Jul 27, 2022

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
Copy link
Contributor Author

ok-nick commented Jul 27, 2022

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
Copy link
Contributor Author

ok-nick commented Jul 27, 2022

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

ok-nick commented Jul 27, 2022

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
Copy link
Contributor Author

ok-nick commented Jul 28, 2022

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
Copy link
Owner

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

Copy link
Owner

@ssalonen ssalonen left a comment

Choose a reason for hiding this comment

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

Review comments. Also, don't forget to update Readme (there is a statement on win support) and changelog

build/build.rs Outdated Show resolved Hide resolved
build/build.rs Outdated Show resolved Hide resolved
build/build.rs Show resolved Hide resolved
build/build.rs Outdated Show resolved Hide resolved
build/build.rs Show resolved Hide resolved
build/build.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@ok-nick ok-nick marked this pull request as ready for review August 2, 2022 00:29
@ok-nick ok-nick requested a review from ssalonen August 2, 2022 00:29
Copy link
Owner

@ssalonen ssalonen left a comment

Choose a reason for hiding this comment

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

One more final comment, then we are good to merge!

build/build.rs Show resolved Hide resolved
build/build.rs Show resolved Hide resolved
@ok-nick ok-nick requested a review from ssalonen August 5, 2022 20:52
@ssalonen
Copy link
Owner

ssalonen commented Aug 6, 2022

Big thank you!

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.

Fails to build on Windows
2 participants