Restore zip/tgz package structure; top-level folder in packages - #617
Open
johguenther wants to merge 2 commits into
Open
Restore zip/tgz package structure; top-level folder in packages#617johguenther wants to merge 2 commits into
johguenther wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
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_MODEblock beforeGNUInstallDirsto controlCMAKE_INSTALL_*DIRlayout. - Update
test_integrationto use${CMAKE_INSTALL_LIBDIR}when settingembree_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.
As the README implies and like the other RenderKit projects have
johguenther
requested review from
stefanatwork and
svenwoop
and
a lite review from Copilot
August 10, 2026 13:50
There was a problem hiding this comment.
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 useNOT 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,$EmbreeDirbecomes 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
v4.4.1 removed CMake presets, which changed the folder structure of release packages; bringing it back in
EMBREE_ZIP_MODE