Skip to content

Use CPack with TARGET_RUNTIME_DLLS for Windows packaging #1897

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.

Sign up for GitHub

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

Open
vulcandth wants to merge 1 commit into
base: master
Choose a base branch
from
+34 −12
Open

Use CPack with TARGET_RUNTIME_DLLS for Windows packaging #1897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/create-release-artifacts.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,12 @@ jobs:
cmake --install build --verbose --prefix install_dir --strip
- name: Package binaries
run: |
Compress-Archive -LiteralPath @("install_dir/bin/rgbasm.exe", "install_dir/bin/rgblink.exe", "install_dir/bin/rgbfix.exe", "install_dir/bin/rgbgfx.exe", "install_dir/bin/z.dll", "install_dir/bin/libpng16.dll") "rgbds-win${{ matrix.bits }}.zip"
cpack --config build/CPackConfig.cmake -G ZIP -C Release --verbose
- name: Upload Windows binaries
uses: actions/upload-artifact@v4
with:
name: win${{ matrix.bits }}
path: rgbds-win${{ matrix.bits }}.zip
path: build/rgbds-win${{ matrix.bits }}.zip

macos:
runs-on: macos-14
Expand Down
6 changes: 4 additions & 2 deletions .github/workflows/testing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,15 @@ jobs:
- name: Package binaries
shell: bash
run: |
cpack --config build/CPackConfig.cmake -G ZIP -C Release --verbose
mkdir bins
cp install_dir/bin/{rgbasm.exe,rgblink.exe,rgbfix.exe,rgbgfx.exe,z.dll,libpng16.dll} bins
cp install_dir/bin/rgb{asm,link,fix,gfx}.exe bins
cp install_dir/bin/*.dll bins
- name: Upload Windows binaries
uses: actions/upload-artifact@v4
with:
name: rgbds-canary-w${{ matrix.bits }}-${{ matrix.os }}
path: bins
path: build/rgbds-win${{ matrix.bits }}.zip
- name: Compute test dependency cache params
id: test-deps-cache-params
shell: bash
Expand Down
22 changes: 21 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,10 @@ else(GIT)
endif(GIT)

find_package(PkgConfig)
if(MSVC OR NOT PKG_CONFIG_FOUND)
if(MSVC)
find_package(ZLIB REQUIRED CONFIG)
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

Why is looking for zlib separately required? I see FindPNG.cmake calls find_package(ZLIB) on my system.

find_package(PNG REQUIRED CONFIG)
Comment on lines +88 to +89
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

CONFIG disables Module-mode search. What is that required for?

elseif(NOT PKG_CONFIG_FOUND)
# fallback to find_package
# cmake's FindPNG is very fragile; it breaks when multiple versions are installed
# this is most evident on macOS but can occur on Linux too
Expand Down Expand Up @@ -132,3 +135,20 @@ foreach(SECTION "man1" "man5" "man7")
set(DEST "${MANDIR}/${SECTION}")
install(FILES ${${SECTION}} DESTINATION ${DEST})
endforeach()

if(WIN32 AND CMAKE_VERSION VERSION_GREATER_EQUAL "3.21")
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

I'm thinking it wouldn't be a bad idea to enable CPack across all platforms. (In particular, it may be useful to generate our releases' source tarballs, though any tweaking related to that can come in a separate PR.)

if(CMAKE_SIZEOF_VOID_P EQUAL 8)
set(CPACK_SYSTEM_NAME "win64")
else()
set(CPACK_SYSTEM_NAME "win32")
endif()
Comment on lines +140 to +144
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

This pointer size check looks cursed as hell, hopefully there's a better way to do this...

EDIT: Turns out that CPACK_SYSTEM_NAME has this exact behaviour by default anyway.

Suggested change
if(CMAKE_SIZEOF_VOID_P EQUAL 8)
set(CPACK_SYSTEM_NAME "win64")
else()
set(CPACK_SYSTEM_NAME "win32")
endif()


set(CPACK_GENERATOR "ZIP")
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

Looks like this gets overridden with the cpack -G flag, so probably we should omit this.

set(CPACK_INCLUDE_TOPLEVEL_DIRECTORY OFF)
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

AFAICT, this is best left undefined so that e.g. tarballs can be properly generated. (It would be necessary to pass it to generate the Windows zip files, but we can do that with -D on the CLI.)

Suggested change
set(CPACK_INCLUDE_TOPLEVEL_DIRECTORY OFF)

set(CPACK_PACKAGE_DIRECTORY "${CMAKE_BINARY_DIR}")
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

This appears to be the default behaviour (though I'd need to check, but I don't have a Windows machine handy.)

Suggested change
set(CPACK_PACKAGE_DIRECTORY "${CMAKE_BINARY_DIR}")

set(CPACK_PACKAGE_FILE_NAME "${PROJECT_NAME}-${CPACK_SYSTEM_NAME}")
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

Suggested change
set(CPACK_PACKAGE_FILE_NAME "${PROJECT_NAME}-${CPACK_SYSTEM_NAME}")
set(CPACK_PACKAGE_FILE_NAME "${PROJECT_NAME}-${CPACK_SYSTEM_NAME}") # We do not put the version number in the release archives.

set(CPACK_INSTALL_CMAKE_PROJECTS "${CMAKE_BINARY_DIR};${CMAKE_PROJECT_NAME};runtime;/")
set(CPACK_STRIP_FILES TRUE)
set(CPACK_VERBATIM_VARIABLES YES)
Comment on lines +151 to +152
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

These two true values are inconsistent, but it looks like we've not been consistent in the rest of our files, so idk.

include(CPack)
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

Apparently we can also set some more variables?

Suggested change
include(CPack)
set(CPACK_PACKAGE_VENDOR "GBDev")
set(CPACK_PACKAGE_DESCRIPTION "An assembly toolchain for the Nintendo Game Boy and Game Boy Color") # Same as our repo's description.
set(CPACK_PACKAGE_HOMEPAGE_URL "https://rgbds.gbdev.io")
set(CPACK_RESOURCE_FILE_LICENSE "LICENSE")
include(CPack)

Also, I'm noticing that this is not setting the CPACK_PACKAGE_VERSION_* variables, and we're not passing RGBDS' version to project(), meaning the fallback CMAKE_PROJECT_VERSION_* variables aren't defined. I'm thinking we may want to extract the Git ref before running project() so that we can extract the version from it and pass that to project().

(This has led me to a rabbit hole of realising how cursed our version setup is, and now I'm thinking of how the hell to fix that. Stay tuned lol)

endif()
10 changes: 6 additions & 4 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ foreach(PROG "asm" "fix" "gfx" "link")
${rgb${PROG}_src}
${common_src}
)
install(TARGETS rgb${PROG} RUNTIME DESTINATION bin)
install(TARGETS rgb${PROG} RUNTIME DESTINATION bin COMPONENT runtime)
# Required to run tests
set_target_properties(rgb${PROG} PROPERTIES
# hack for MSVC: no-op generator expression to stop generation of "per-configuration subdirectory"
Expand All @@ -122,9 +122,11 @@ if(LIBPNG_FOUND) # pkg-config
target_link_directories(rgbgfx PRIVATE ${LIBPNG_LIBRARY_DIRS})
target_link_libraries(rgbgfx PRIVATE ${LIBPNG_LIBRARIES})
else()
target_compile_definitions(rgbgfx PRIVATE ${PNG_DEFINITIONS})
target_include_directories(rgbgfx PRIVATE ${PNG_INCLUDE_DIRS})
target_link_libraries(rgbgfx PRIVATE ${PNG_LIBRARIES})
target_link_libraries(rgbgfx PRIVATE PNG::PNG)
endif()

if(WIN32 AND CMAKE_VERSION VERSION_GREATER_EQUAL "3.21")
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

Why is a version check required?

install(FILES "$<TARGET_RUNTIME_DLLS:rgbgfx>" DESTINATION bin COMPONENT runtime)
endif()

include(CheckLibraryExists)
Expand Down
4 changes: 1 addition & 3 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ foreach(TARGET randtilegen rgbgfx_test)
target_link_directories(${TARGET} PRIVATE ${LIBPNG_LIBRARY_DIRS})
target_link_libraries(${TARGET} PRIVATE ${LIBPNG_LIBRARIES})
else()
target_compile_definitions(${TARGET} PRIVATE ${PNG_DEFINITIONS})
target_include_directories(${TARGET} PRIVATE ${PNG_INCLUDE_DIRS})
target_link_libraries(${TARGET} PRIVATE ${PNG_LIBRARIES})
target_link_libraries(${TARGET} PRIVATE PNG::PNG)
Copy link
Member

@ISSOtm ISSOtm Mar 9, 2026

Choose a reason for hiding this comment

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

Oh, very nice, thank you! We should do the same for the pkg-config path, tbh.

endif()
endforeach()

Expand Down
Loading