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

Auto-package windows binaries on build (and fix a few bugs and warnings) #1391

Conversation

dmaltsiniotis
Copy link
Contributor

@dmaltsiniotis dmaltsiniotis commented Dec 18, 2023

This PR adds a new windows build package artifact to each build, and also addresses a number of windows-related build issues.

The primary change with this pull request is the addition of a new artifact on build called hackrf-tools-windows.zip. This artifact contains the combination of the windows libhackrf build and hackrf-tools build together. The intent of this artifact is to provide a convenient way for Windows users who prefer to download pre-built Windows binaries to do so from an official source. This is the first step in a longer-term vision to be able to auto-package the HackRF One libraries and tools around a more comprehensive installer.

Specific changes in this pull request are:

  • Add artifact publish step for Windows libraries and host tools.
  • Replace runner.workspace with github.workspace to fix inconsistent pathing issues on the build server.
  • Update checkout and uploadartifact tasks to v4 to suppress warnings since v3 uses an end-of-life node version 16.
  • Update CMAKE for libhackrf and hackrf-tools to also install DLLs on WIN32 platforms.
  • Add a new windows-only environment variable for the vcpkg msbuild cmake toolchain makefile.

Comments and criticisms are welcome.

@martinling
Copy link
Member

@dmaltsiniotis what are the remaining issues with this? It seems like it's working?

@dmaltsiniotis
Copy link
Contributor Author

dmaltsiniotis commented Feb 22, 2024

@dmaltsiniotis what are the remaining issues with this? It seems like it's working?

Hi @martinling,

Thanks for reminding me on this PR.

The last issue that I ran into with this PR is that there is a discrepancy between running CMAKE locally on Windows, vs running CMAKE on the Windows build agents in GitHub. The difference is that when I run the CMAKE install locally, the 3rd party dependencies installed by vcpkg install (libusb, pthreads, fftw) are copied over into the install directory. However when cmake install runs in GitHub, the three libraries are NOT copied over to the install directory, resulting in an incomplete zip package.

I have not been successful in coaxing CMAKE to force copy the DLLs over every time, but my knowledge if CMAKE is rather rudimentary.

That is the last remaining issue, otherwise the builds and the new packaging step all run successfully.

@martinling
Copy link
Member

Thanks for clarifying! What an annoying little problem to be left with.

I've had some success at using action-tmate to get a shell on the GitHub CI runners and debug things interactively that way, might be worth trying here.

But sometimes the result is just "it behaves differently on the runner and I can't tell why!".

@martinling
Copy link
Member

@dmaltsiniotis I've just had a go at building the release branch (shortly to be 2024.02.1) on Windows 10 following the same steps as we use in CI, using:

  • vcpkg at commit 62aa4492
  • CMake 3.28.3
  • Visual Studio Community 2022 (minimal install with just MSVC v143, MSBuild, C runtime and W10 SDK)

I get the same result you see: everything is built correctly but the external DLL dependencies aren't copied to the ${CMAKE_INSTALL_PREFIX}/bin directory.

@dmaltsiniotis dmaltsiniotis force-pushed the feature/package-windows-artifacts branch from aef2a05 to 0fcc86c Compare February 29, 2024 23:45
@dmaltsiniotis dmaltsiniotis changed the title DRAFT: Not yet ready. Auto-package windows binaries on build. DRAFT: Auto-package windows binaries on build (and fix a few bugs and warnings) Feb 29, 2024
@dmaltsiniotis dmaltsiniotis changed the title DRAFT: Auto-package windows binaries on build (and fix a few bugs and warnings) Auto-package windows binaries on build (and fix a few bugs and warnings) Feb 29, 2024
@dmaltsiniotis dmaltsiniotis marked this pull request as ready for review March 1, 2024 00:25
@dmaltsiniotis
Copy link
Contributor Author

dmaltsiniotis commented Mar 1, 2024

@martinling, I think I've finally addressed the missing dll mystery. This PR is ready to review now.

I haven't found the exact cause of why it doesn't work specifically on the build server, but the issue is that the vcpkg makefile is not being found/invoked on the build server, but it is on my local instance (environment variable difference?). The solution was to force/hint CMAKE to find it by specifying the vcpkg msbuild makefile as an additional tool-chain via command line parameter. Alternatively, we could include the vcpkg cmake file as part of an include() in the CMakeLists.txt (see: https://stackoverflow.com/questions/55496611/cmake-cannot-find-libraries-installed-with-vcpkg)

…p hackrf-tools.

* Add artifact publish step for Windows libraries and host tools.
* Replace runner.workspace with github.workspace.
* Update checkout task to v4 to supress warning.
* Update CMAKE for libhackrf and hackrf-tools to also install DLLs on WIN32 platforms.
* Update uploadartifact task to v4 since v3 uses an end-of-life node version 16.
* Add a new windows variable for the vcpkg cmake toolchain makefile.
@dmaltsiniotis dmaltsiniotis force-pushed the feature/package-windows-artifacts branch from 0fcc86c to 3d83a2f Compare March 1, 2024 00:39
@dmaltsiniotis dmaltsiniotis changed the title Auto-package windows binaries on build (and fix a few bugs and warnings) Issue #1286: Auto-package windows binaries on build (and fix a few bugs and warnings) Mar 1, 2024
Copy link
Member

@martinling martinling left a comment

Choose a reason for hiding this comment

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

Great!

I agree with specifying the CMAKE_TOOLCHAIN_FILE on the command line in the CI script, since it's specific to where that file is installed on the build server. I wouldn't want to hardcode that in the CMakeLists.txt.

At some point I would like to see the package-finding code in our CMake setup get smart enough to find the vcpkg-installed dependencies without having to give all the paths explicitly, but that's out of the scope of this PR.

Thanks for all your work on this, it's very much appreciated.

@martinling martinling requested a review from mossmann March 1, 2024 11:16
@mossmann mossmann changed the title Issue #1286: Auto-package windows binaries on build (and fix a few bugs and warnings) Auto-package windows binaries on build (and fix a few bugs and warnings) Mar 7, 2024
@mossmann
Copy link
Member

mossmann commented Mar 7, 2024

Fixes #1286

@martinling martinling linked an issue Mar 7, 2024 that may be closed by this pull request
@mossmann mossmann merged commit 1525b62 into greatscottgadgets:master Mar 7, 2024
17 checks passed
@mossmann
Copy link
Member

mossmann commented Mar 7, 2024

Thank you very much for the contribution!

@dmaltsiniotis dmaltsiniotis deleted the feature/package-windows-artifacts branch March 7, 2024 17:12
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.

Alternative Windows host tools binary download source
3 participants