Skip to content
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.

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

Revert "cmake: fix breaking build tree on version/revision bumps" #198775

Closed
wants to merge 1 commit into from

Conversation

akretz
Copy link
Contributor

@akretz akretz commented Nov 23, 2024

This backports https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9987 to 3.31.1

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Nov 23, 2024
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Nov 23, 2024
Copy link
Contributor

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

@SMillerDev
Copy link
Member

Can we use the patch that was merged upstream instead?

@akretz
Copy link
Contributor Author

akretz commented Nov 24, 2024

Can we use the patch that was merged upstream instead?

That's possible. I've tried to apply the fix
Kitware/CMake@87cb303...0994bc7
directly on 3.31.1, but it failed with conflicts. So I've resolved them and pushed it onto my fork. I've also had to cherry-pick the helper function
Kitware/CMake@2503b43
which it depends on.

@akretz
Copy link
Contributor Author

akretz commented Nov 24, 2024

Actually, the upstream patch also fails on my minimal example of #198772, so I removed it again. The interesting thing is that exactly the same patch works when I build CMake not using brew. So right now the situation with regards to that minimal example is

3.31.1 3.31.1 patched
Build independently
Build brew formula

Probably @carlocab has a better insight on what is going on.

@carlocab
Copy link
Member

Actually, the upstream patch also fails on my minimal example of #198772, so I removed it again.

If I'm understanding this correctly: it seems like this needs to be reported upstream then. Because we can revert #197088 now, but that's just going to break again when upstream changes regarding symlink handling land in a CMake release.

Can you try building cmake from their master branch to double-check that you see the problem from #198772 there?

@akretz
Copy link
Contributor Author

akretz commented Nov 24, 2024

Can you try building cmake from their master branch to double-check that you see the problem from #198772 there?

I have now built upstream master 1 using ./bootstrap --prefix=/opt/homebrew/Cellar/cmake/master. The situation with #198772 is as follows:

brew install cmake
cmake .. # fails
/opt/homebrew/bin/cmake .. # fails
/opt/homebrew/Cellar/cmake/3.31.1/bin/cmake .. # succeeds
brew unlink cmake
ln -s ../Cellar/cmake/master/bin/cmake /opt/homebrew/bin/cmake
cmake .. # succeeds

I have started from a clean build folder with each cmake invokation.

Footnotes

  1. (Kitware/CMake@51f43b6)

@carlocab
Copy link
Member

Great, can we backport https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9987?

@akretz
Copy link
Contributor Author

akretz commented Nov 24, 2024

Great, can we backport https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9987?

I've done that now and put the diff inside the formula in this PR.

For some reason, this still fails on #198772 when I install it via brew. But when I build exactly the same status via

./bootstrap --prefix=/opt/homebrew/Cellar/cmake/master
make
make install
ln -s ../Cellar/cmake/master/bin/cmake /opt/homebrew/bin/cmake

it works. I'm not sure what brew is doing here that breaks it when I build via brew.

@akretz
Copy link
Contributor Author

akretz commented Nov 24, 2024

I have figured out what is happening now:

  • The Eigen package provides an Eigen3Config.cmake as well as a deprecated FindEigen3.cmake
  • Ceres doesn't play well with FindEigen3.cmake
  • Previously, CMake's discovery algorithm found Eigen3Config.cmake to set the CMake variables accordingly
  • Now, with CMake's algorithm starting from /opt/homebrew/bin/cmake, it finds /opt/homebrew/share/cmake/Modules/FindEigen3.cmake instead
  • I didn't pass --datadir to the configure script on my build of CMake, so it still found Eigen3Config.cmake

So it's not an issue with CMake. I propose deleting

(share/"cmake/Modules").install "cmake/FindEigen3.cmake"

to fix this issue, because it has been deprecated by Eigen. The file has been deleted on their master branch.

@carlocab Is it okay if I open another PR for that one? Feel free to close or merge this PR.

@cho-m
Copy link
Member

cho-m commented Nov 24, 2024

EDIT: Didn't see @akretz comment before posting. Same analysis.


Partly an issue in Eigen as current Eigen installs both FindEigen3.cmake (module) and Eigen3Config.cmake (config). Modules should be removed in future - https://gitlab.com/libeigen/eigen/-/commit/f2984cd0778dd0a1d7e74216d826eaff2bc6bfab

CMake defaults to searching module before config. With realpath handling, this means it would only pick modules installed by CMake. After no longer resolving realpath, this means it picks up modules installed by other formulae and linked into share/cmake/Modules (like FindEigen3.cmake).

The problem is FindEigen3.cmake adds a default version number (2.91.0) which causes havoc in the package lookup.

Can see this by trying workarounds like:

find_package(Eigen3 3.4.0)
find_package(Eigen3 CONFIG)

@carlocab
Copy link
Member

I'm not sure what brew is doing here that breaks it when I build via brew.

That's because it isn't sufficient to symlink only bin/cmake to HOMEBREW_PREFIX/bin to replicate what brew is doing -- you need to symlink everything in Cellar/cmake/master into HOMEBREW_PREFIX. This will change the behaviour of CMake, as you've observed.

Is it okay if I open another PR for that one?

This seems fine to me.

@akretz akretz closed this Nov 27, 2024
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.

4 participants