Skip to content

Fix: CMake LTO Config#716

Open
xsscx wants to merge 7 commits intomasterfrom
fix-cmake-lto
Open

Fix: CMake LTO Config#716
xsscx wants to merge 7 commits intomasterfrom
fix-cmake-lto

Conversation

@xsscx
Copy link
Member

@xsscx xsscx commented Mar 25, 2026

Fix: CMake LTO Config
(#715)

Pull Request Checklist

  • Have you followed the guidelines in Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you built your Pull Request locally with the Build Instructions?
  • Have you added or updated relevant tests?
  • Have you added or updated relevant docs?

Fix: CMake LTO Config
@xsscx xsscx self-assigned this Mar 25, 2026
@xsscx xsscx added PR Pull Request Test Status Maintainer indicates TEST Status labels Mar 25, 2026
@xsscx xsscx linked an issue Mar 25, 2026 that may be closed by this pull request
D Hoyt and others added 6 commits March 25, 2026 10:16
Changes to ci-comprehensive-build-test.yml:

1. Add fix-cmake-lto to branch dropdown options
2. Fix validate-ref to accept valid branch names (not just master + PR refs)
3. Add 8 LTO/IPO matrix entries testing all CMakeLists.txt branches:
   - User override OFF/ON (should be respected unconditionally)
   - Auto-enable for Release builds
   - Auto-disable for Debug/RelWithDebInfo
   - Auto-disable when ASAN or Coverage enabled
   - User override wins over sanitizer auto-disable
4. Add LTO validation step: captures cmake output, greps for status
   messages, validates CMakeCache values match expectations
5. Fix Windows double build (duplicated cmake --build command)
6. Fix summary: replace hardcoded checkmarks with result_icon() that
   maps actual job results to emoji (success/failure/cancelled/skipped)
7. Normalize sed patterns: use sed -i.bak and preserve indentation
   in commented-out lines across all 5 occurrences

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CMakeLists.txt uses set(CMAKE_INTERPROCEDURAL_OPTIMIZATION ON) without
CACHE for auto-enable cases, so the variable does NOT appear in
CMakeCache.txt. Only user -D overrides create cache entries.

Restructured validation:
- Primary: cmake status message (works for all cases)
- Secondary: CMakeCache check (only for user-override cases)
- Tertiary: CMakeFiles property propagation check (informational)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The second cmake --build invocation is intentional — IccXML2.dll import
library must be generated before iccFromXml/iccToXml can link against it.
Without it, LNK1181 fatal error on Release builds.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Root cause: shared libraries (IccProfLib2.dll, IccXML2.dll) have no
__declspec(dllexport) annotations — MSVC generates .dll but NO .lib
import library. A POST_BUILD workaround copies the static .lib as an
alias, but MSBuild's parallel build races: tools start linking before
the alias is created, causing LNK1181.

Fixes:
- Add WINDOWS_EXPORT_ALL_SYMBOLS ON to both shared library targets
  so MSVC generates proper import libraries alongside DLLs
- Fix tool dependencies (IccFromXml, IccToXml): when both shared and
  static libs are enabled, depend on BOTH targets to ensure the
  POST_BUILD alias completes before the tool link phase

This eliminates the need for a second deterministic build pass on
Windows MSVC to resolve link ordering failures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The ref input defaulted to 'master' (choice type), which is truthy and
always takes precedence over github.ref_name in the CHECKOUT_REF
expression. Changed to string type with empty default so --ref flag
naturally flows through the || chain.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cmake -E __create_def exits with 0xC0000005 (ACCESS_VIOLATION) when
scanning IccProfLib2 object files on GitHub Actions Windows runners.
Revert to the proven static alias + second build pass approach.
Dependency ordering fix (IccFromXml/IccToXml → both shared+static)
is retained as defense-in-depth.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@xsscx xsscx added Merge Ready Maintainer indicates Merge Ready and removed Test Status Maintainer indicates TEST Status pending labels Mar 25, 2026
@xsscx xsscx marked this pull request as ready for review March 25, 2026 15:42
@xsscx xsscx requested review from ChrisCoxArt and dwtza as code owners March 25, 2026 15:42
@xsscx xsscx removed the request for review from dwtza March 25, 2026 15:42
@xsscx xsscx added Pending Merge Maintainer indicates Merge Pending and requests no further changes and removed Merge Ready Maintainer indicates Merge Ready pending labels Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Pending Merge Maintainer indicates Merge Pending and requests no further changes PR Pull Request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix: CMake LTO Config

2 participants