Revert "[CMake] Remove overriding CMAKE_OSX_SYSROOT"

[vepadulano]

This reverts commit e3a10ab.

The MacOS builds on the CI were working by chance as shown at #19854.

[devajithvs]

We still need to figure out why this is happening and what was happening before CMake 4. But let's get this in to not break the builds.

[guitargeek]

Can we make sure it also works in the case where xcrun is not available? Then we don't have to re-introduce patches for Nix or other ways to build ROOT on osx that are independent of XCode. I mean just skipping this code block if xcrun can't be found

[github-actions]

Test Results

    20 files      20 suites   3d 14h 48m 9s ⏱️
 3 663 tests  3 611 ✅   0 💤 52 ❌
71 547 runs  71 131 ✅ 363 💤 53 ❌

For more details on these failures, see this check.

Results for commit 4fc2ee9.

♻️ This comment has been updated with latest results.

[guitargeek]

Also, if the reason that is was not necessary in our CI to set CMAKE_OSX_SYSROOT was that SDKROOT was already set in our CI, can we make sure that our CI doesn't do that before merging this? Our CI configuration should not set any secret CMake variables that are necessary to compile. IMO, we should have a configuration similar to the standard user configuration, so that we are sensitive to possible problems with it.

[vepadulano]

Also, if the reason that is was not necessary in our CI to set CMAKE_OSX_SYSROOT was that SDKROOT was already set in our CI, can we make sure that our CI doesn't do that before merging this? Our CI configuration should not set any secret CMake variables that are necessary to compile. IMO, we should have a configuration similar to the standard user configuration, so that we are sensitive to possible problems with it.

As I said in my comment above, I have no idea who/what is messing with our CI runners so that the SDKROOT variable is set. I unfortunately don't have time to investigate that. I refuse to block this PR because of that, because that means forcing any dev compiling ROOT master on MacOS to also include this commit reverting the change and that is not sustainable

[guitargeek]

Which comment above? Maybe you mean the private discussion on mattermost 😃 Or did I miss something?

Yes sure, no need to block this PR for this, but can we maybe write at least some notes? So if this code is touched again in the future it's clear what the constraints are and that there is something in the CI that can be improved about not setting the SDKROOT. A TODO in the touched CMake code is the best IMHO because the most visible. Commit message or PR description is also fine, but I think in-code comments are less likely to be missed.

[guitargeek]

Ah I see now, the information is indirectly available via the link to #19854 ! Okay that's maybe alright, but I think directly having it in the code comments or commit messages will be better.

Sorry for being pedantic, but we need to minimize the risk that people like me break things in the future again because they are missing the context 🙂

[guitargeek]

Thanks for considering my request about checking if xcrun is available 🙏

Good now for me, maybe only consider an inline note on why this CMake code block is seemingly not needed in our CI. And that it would be better to avoid setting implicitly required CMake variables in the CI.

[vepadulano]

Good now for me, maybe only consider an inline note on why this CMake code block is seemingly not needed in our CI. And that it would be better to avoid setting implicitly required CMake variables in the CI.

Yes, done in the latest commit 👍