Skip to content

Restore zip/tgz package structure; top-level folder in packages - #617

Open
johguenther wants to merge 2 commits into
masterfrom
guj/fixes
Open

Restore zip/tgz package structure; top-level folder in packages#617
johguenther wants to merge 2 commits into
masterfrom
guj/fixes

Conversation

@johguenther

Copy link
Copy Markdown
Contributor

v4.4.1 removed CMake presets, which changed the folder structure of release packages; bringing it back in EMBREE_ZIP_MODE

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Restores Embree release zip/tgz package layout behavior by pinning install directory variables in EMBREE_ZIP_MODE, and updates the integration packaging test to locate the installed CMake package via ${CMAKE_INSTALL_LIBDIR}.

Changes:

  • Add an EMBREE_ZIP_MODE block before GNUInstallDirs to control CMAKE_INSTALL_*DIR layout.
  • Update test_integration to use ${CMAKE_INSTALL_LIBDIR} when setting embree_DIR.
  • Adjust the file header comment (copyright line).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread common/cmake/package.cmake
Comment thread common/cmake/package.cmake
As the README implies and like the other RenderKit projects have
@johguenther johguenther changed the title Restore zip/tgz package structure Restore zip/tgz package structure; top-level folder in packages Aug 10, 2026
@johguenther
johguenther requested review from stefanatwork and svenwoop and a lite review from Copilot August 10, 2026 13:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (6)

common/cmake/package.cmake:2

  • This header change drops the year range used elsewhere in common/cmake (most files use "2009-2021" or later). If this wasn’t intentional, it should be restored/updated for consistency and correctness of the copyright notice.
## Copyright 2009 Intel Corporation
## SPDX-License-Identifier: Apache-2.0

superbuild/CMakeLists.txt:43

  • The install-dir overrides (CMAKE_INSTALL_{BIN,LIB,INCLUDE,DOC}DIR) are applied unconditionally in the superbuild, which changes the default GNUInstallDirs behavior (e.g., forcing lib instead of lib64 on some 64-bit Linux setups). That can break consumers that rely on the platform-default layout when not producing ZIP-style packages. Consider gating this block behind an explicit toggle (e.g., EMBREE_ZIP_MODE) so the flat layout is only forced when intended.
# Pin the flat package layout before GNUInstallDirs
if (NOT DEFINED CMAKE_INSTALL_BINDIR)
  set(CMAKE_INSTALL_BINDIR "bin")
endif()
if (NOT DEFINED CMAKE_INSTALL_LIBDIR)
  set(CMAKE_INSTALL_LIBDIR "lib")
endif()
if (NOT DEFINED CMAKE_INSTALL_INCLUDEDIR)
  set(CMAKE_INSTALL_INCLUDEDIR "include")
endif()
if (NOT CMAKE_INSTALL_DOCDIR)
  set(CMAKE_INSTALL_DOCDIR "doc")
endif()

common/cmake/package.cmake:17

  • IF (NOT CMAKE_INSTALL_DOCDIR) will also evaluate true when the variable is intentionally set to an empty string, causing this block to overwrite a caller’s explicit choice. The other install-dir pins use NOT DEFINED; DOC should be consistent.
  IF (NOT CMAKE_INSTALL_DOCDIR)
    SET(CMAKE_INSTALL_DOCDIR "doc")
  ENDIF()

scripts/prepare-test-files.sh:77

  • EMBREE_DIR=$(echo "$DEST_DIR"/embree-*) is fragile: if the glob matches multiple directories (or none), EMBREE_DIR becomes a space-separated list (or the literal pattern), which breaks downstream steps. Prefer resolving exactly one match and failing fast otherwise.
  # archives have a top level directory named after the package
  EMBREE_DIR=$(echo "$DEST_DIR"/embree-*)
  echo "EMBREE_DIR=$EMBREE_DIR" >> "$GITHUB_ENV"
  echo "$EMBREE_DIR"  # Output for immediate use

scripts/prepare-test-files-macos.sh:77

  • EMBREE_DIR=$(echo "$DEST_DIR"/embree-*) is fragile: if the glob matches multiple directories (or none), EMBREE_DIR becomes a space-separated list (or the literal pattern), which breaks downstream steps. Prefer resolving exactly one match and failing fast otherwise.
  # archives have a top level directory named after the package
  EMBREE_DIR=$(echo "$DEST_DIR"/embree-*)
  echo "EMBREE_DIR=$EMBREE_DIR" >> "$GITHUB_ENV"
  echo "$EMBREE_DIR"  # Output for immediate use

scripts/prepare-test-files.ps1:47

  • If no embree-* directory exists under $DestDir, $EmbreeDir becomes empty and the workflow continues with an invalid EMBREE_DIR. Add an explicit check and fail early so CI errors are easier to diagnose.
    # archives have a top level directory named after the package
    $EmbreeDir = (Get-ChildItem -Path $DestDir -Directory -Filter "embree-*" | Select-Object -First 1).FullName
    "EMBREE_DIR=$EmbreeDir" | Out-File -FilePath $env:GITHUB_ENV -Append
    Write-Output $EmbreeDir  # Output for immediate use

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.

2 participants