Skip to content

Support MinGW#139

Open
Agapanthus wants to merge 5 commits intoZEISS:mainfrom
Agapanthus:mingw
Open

Support MinGW#139
Agapanthus wants to merge 5 commits intoZEISS:mainfrom
Agapanthus:mingw

Conversation

@Agapanthus
Copy link

STOP - Read this First!

Reporting a security vulnerability?
Check out the project's security policy.

Fill out and Adjust this Template

Description

  • Change #include <Windows.h> to #include <windows.h>.
    • With MSVC, headers are case-insensitive, but with MinGW, only lowercase is accepted.
  • Additionally, support for Windowscodecs is limited when cross-compiling.
    • A new option, LIBCZI_HAVE_WINCODECS_API, has been added to only enable this feature when compiling for Windows on Windows.
    • When disabled, libczi will not link against Windowscodecs, and the CWicJpgxrDecoder will be excluded from compilation.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • I have successfully built libczi using MSVC, GCC, and GCC-mingw.

Checklist:

  • I followed the Contributing Guidelines.
  • I did a self-review.
  • I commented my code, particularly in hard-to-understand areas.
  • I updated the documentation.
  • I updated the version of libCZI following Versioning of libCZI depending on the type of change
    • Bug fix -> PATCH
    • New feature -> MINOR
    • Breaking change -> MAJOR
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

@ptahmose ptahmose requested a review from Copilot July 4, 2025 21:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Enable MinGW compatibility by normalizing Windows header casing and conditionally enabling Windowscodecs support when not cross-compiling.

  • Switch all #include <Windows.h> to lowercase <windows.h> for MinGW.
  • Introduce LIBCZI_HAVE_WINCODECS_API to guard Windowscodecs features and adjust CMake linking.
  • Exclude the JPEG-XR decoder (CWicJpgxrDecoder) when Windowscodecs is disabled.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Src/libCZI/utilities.cpp Lowercased <Windows.h> include.
Src/libCZI/libCZI_Site.cpp Wrapped JPXR decoder case in #if LIBCZI_HAVE_WINCODECS_API.
Src/libCZI/decoder_wic.h Added LIBCZI_HAVE_WINCODECS_API to include guard.
Src/libCZI/decoder_wic.cpp Updated implementation guard to require new macro.
Src/libCZI/StreamsLib/windowsfileinputstream.h Lowercased <Windows.h> include.
Src/libCZI/StreamImpl.h Lowercased <Windows.h> include.
Src/libCZI/CMakeLists.txt Split Windowscodecs linking under new config flag.
CMakeLists.txt Bumped version to 0.66.1 and defined LIBCZI_HAVE_WINCODECS_API.
Src/JxrDecode/jxrlib/image/encode/strenc.c Lowercased commented-out windows.h include.
Src/CZICmd/utils.cpp Lowercased <Windows.h> include.
Src/CZICmd/CZIcmd.cpp Lowercased <Windows.h> include.
Src/CZICmd/BitmapGenGdiplus.h Lowercased <Windows.h> include.
Comments suppressed due to low confidence (2)

CMakeLists.txt:89

  • Please update the project documentation (e.g., README or docs) to describe the new LIBCZI_HAVE_WINCODECS_API option and its effect on cross-compilation.
# Determine whether we are cross-compiling to Windows. When cross-compiling, there is insufficient support

Src/libCZI/libCZI_Site.cpp:62

  • Add unit tests to cover both branches of LIBCZI_HAVE_WINCODECS_API, ensuring the JPEG-XR decoder is correctly included or excluded.
#if LIBCZI_HAVE_WINCODECS_API

@codecov
Copy link

codecov bot commented Jul 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.35%. Comparing base (494ac62) to head (53db5e8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #139      +/-   ##
==========================================
+ Coverage   66.08%   67.35%   +1.26%     
==========================================
  Files          88       87       -1     
  Lines       11265    11053     -212     
==========================================
  Hits         7445     7445              
+ Misses       3820     3608     -212     
Flag Coverage Δ
windows-latest 67.35% <ø> (+1.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ptahmose
Copy link
Contributor

@Agapanthus
Thanks for this PR and the interest in libCZI and libCZIAPI.
Please note that before we can accept your, we need to have a signed CLA from you. Please check here. A photo with a phone of the signed form is sufficient.

@m-aXimilian
Copy link
Contributor

@Agapanthus
Please also update the version_history.md documenting your enhancement. Something like "add MinGW support" maybe?

@Agapanthus
Copy link
Author

Thank you for your feedback and sorry for the delay. I've now updated the version history and sent you the CLA.

@ptahmose ptahmose added the cla Contributor License Agreement sent to Admin label Aug 4, 2025
@ptahmose
Copy link
Contributor

ptahmose commented Aug 5, 2025

Thanks for the CLA.

Some initial thoughts on your PR:

  • I'd be reluctant to change to lowercase "windows.h", because at least on Windows (with Windows-SDK), it actually is "Windows.h". Yes, it doesn't matter on Windows, but I'd have a bad conscience to use "wrong spelling" here by intention. Would it maybe makes sense to check for include files (within CMake) and make this a configure-time setting? Are you sure that it is lowercase in all MingW-versions and scenarios?
  • When checking for availability of "Windowscodecs.lib" I'd tend to also prefer a configure-time check (maybe CheckLibraryExists ?).
  • The build-system of libCZI currently is not really designed for cross-compilation scenarios. One has to tweak settings so that the configure-time compile-tests do not run, something like
        # for cross-compilation scenarios, prevent execution of test-programs inside the libCZI-build-scripts
        -DCRASH_ON_UNALIGNED_ACCESS=FALSE
        -DIS_BIG_ENDIAN=FALSE

It would be great to have this improved, or at least documented.

  • Also - it would be great to have a MingW-cross-compilation-scenario as part of the GH-action (i.e. being added to the respective yml).

#include <libCZI_Config.h>

#if LIBCZI_WINDOWSAPI_AVAILABLE
#if LIBCZI_WINDOWSAPI_AVAILABLE && LIBCZI_HAVE_WINCODECS_API
Copy link
Contributor

@ptahmose ptahmose Aug 5, 2025

Choose a reason for hiding this comment

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

I don't see how this define would be propagated into the compilation. I'd don't see that the libCZI_Config.h.in mechanism is used - which would be the preferred way I guess. And I don't see it otherwise passed to the compiler (e.g. with target_compile_definitions).
Am I missing something here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla Contributor License Agreement sent to Admin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants