Skip to content

Avoid using patched MacOS modulemap on conda-forge builds #22448

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
vepadulano wants to merge 1 commit into
base: master
Choose a base branch
from
+11 −1
Open

Avoid using patched MacOS modulemap on conda-forge builds #22448

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
12 changes: 11 additions & 1 deletion interpreter/cling/lib/Interpreter/CIFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,17 @@ namespace {
/*AllowModulemapOverride=*/ false);
#elif __APPLE__
if (Triple.isMacOSX()) {
if (CI.getTarget().getSDKVersion() < VersionTuple(14, 4)) {
// The following is needed to support MacOS13 with LLVM 18, but does not
// make sense in a conda build where the the SDK version is much older
// (typically 11) but the modulemap is shipped separately by conda-forge.
const bool isCondaBuild = []() {
if (const auto en = std::getenv("CONDA_BUILD")) {
return std::strcmp(en, "1") == 0;
}
return false;
}();
Comment on lines +735 to +740
Copy link
Copy Markdown
Member

@hahnjo hahnjo Jun 1, 2026

Choose a reason for hiding this comment

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

IMO this hack should definitely not become part of the generic Cling. I know we discussed in the past, I forgot the details, but if the SDK version is not correctly found it means that some flags are not correct. I don't know where is the proper place to fix it.

Copy link
Copy Markdown
Member Author

@vepadulano vepadulano Jun 1, 2026

Choose a reason for hiding this comment

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

In this case it's more subtle than that. The SDK is properly found by the build, thanks to the now fixed and well-defined CMAKE_OSX_SYSROOT variable https://github.com/root-project/root/blob/dd9c2d39dda88e817002130bbebb752f7fa94eec/cmake/modules/SetOSX_SDK.cmake . That works now.

These lines in CIFactory.cpp though are actively going against the already-found and correct MacOS SDK path. That's because on conda-forge the MacOS SDK used is version 11.0. These lines in CIFactory.cpp are triggered because of the condition <14.2, but the patched modulemap provided in this case is simply wrong. If these lines were not present, the correct CMake workflow would run.

if (CI.getTarget().getSDKVersion() < VersionTuple(14, 4) &&
Copy link
Copy Markdown
Contributor

@devajithvs devajithvs Jun 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

What is the value of CI.getTarget().getSDKVersion() for conda builds?

If the SDK version is not correctly found, I think this value is empty. Then maybe a simpler solution would be to default to std_darwin.modulemap, by saying

if (!CI.getTarget().getSDKVersion().empty() && CI.getTarget().getSDKVersion() < VersionTuple(14, 4))

EDIT: Just read the comment above. The value is 11, I take back my comment.

Copy link
Copy Markdown
Member Author

@vepadulano vepadulano Jun 3, 2026

Choose a reason for hiding this comment

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

Indeed, to be explicit, the following line added in CIFactory.cpp:

std::cout << "MacOS SDK version: " << CI.getTarget().getSDKVersion().getAsString()  << std::endl;

Gives during the conda build:

MacOS SDK version: 11.0

!isCondaBuild) {
maybeAppendOverlayEntry(stdIncLoc.str(),
"std_darwin.MacOSX14.2.sdk.modulemap",
clingIncLoc.str().str(), MOverlay,
Expand Down
Loading