InsightSoftwareConsortium
Update to newer eigen when available
Description
Warnings related to unused eigen symbols are being reported since a while on some of the platforms builds, e.g. https://open.cdash.org/viewBuildError.php?type=1&buildid=8095196
All RogueResearch sites, which use Apple Clang, show these build warnings.
Steps to Reproduce
- Download ITK
- Configure
- Build using Apple Clang
Expected behavior
No warnings are present when building ITK.
Actual behavior
A few build warnings are reported when compiling ITK with Apple Clang.
Reproducibility
%100
Versions
ITK master.
Environment
OS: macOS (different versions: 10.x, 11, 12) CMake: (?) Compiler: Apple Clang
Additional Information
Related to: https://github.com/InsightSoftwareConsortium/ITK/commit/9d9b4165c3fd66746d9c77ed5804190afac817dc
The MR https://gitlab.com/libeigen/eigen/-/merge_requests/864 related to the referenced issue https://gitlab.com/libeigen/eigen/-/issues/2416 was merged to eigen master on March, 3 2022
Last eigen update in ITK: https://github.com/InsightSoftwareConsortium/ITK/pull/3133 (eigen 3.4).
Unfortunately, there hasn't been yet a new release since then (August 18, 2021)
Relevant information: https://gitlab.com/libeigen/eigen/-/milestones https://listengine.tuxfamily.org/lists.tuxfamily.org/eigen/2021/09/msg00000.html
Those eigen warnings are still on cdash. IIRC I fixed some of them upstream in eigen itself.
It looks like their last release is still 3.4 from 2021: https://gitlab.com/libeigen/eigen/-/releases
I found a ticket pushing for a new release: https://gitlab.com/libeigen/eigen/-/issues/2699
Thanks for the follow-up Sean. It looks like the library gets commits regularly; not sure why no new releases have been cut.
Given the lack of Eigen releases, another option is to update ITK to use Eigen master (we ping/freeze the commit we use). This would be a major upgrade of Eigen, but we don't have much code using it, and we don't use any fancy feature, so it's doable to keep up to date with upstream.
This way we can also fix warning upstream rapidly. Now, we have to backport them to 3.4.
@phcerdan sounds like a good plan to me. Is someone familiar with syncing to eigen?
Opened PR #4265 updating to eigen master. Hitting long-existing issue when updating ThirdParty modules, but I think the update went well, but still work in progress.
@N-Dekker realized that there is a 3.4.1 branch in #4265.
It seems promising, maybe they are working on a new release. The current last release was from years ago.
Not sure, good to discuss what we prefer, master or this 3.4.1 branch.
I tend to prefer master, living at the head philosophy. It might be a bit bumpy at the start, but we can solve our warnings way faster this way.
But beyond that, I am open to whatever we decide! Pinging @seanm @thewtex @jhlegarreta @dzenanz.
In general, I'm in favor of using a tagged version (of any library), as it is usually more stable, more carefully maintained, tested, and documented than just any arbitrary revision from the main/master branch. So I don't really embrace that so-called "living at the head philosophy". Maybe for experimental software, but not for a stable ITK release. What do you think?
PS If there is a specific reason for a specific third party library to use a particular revision from its commit history, instead of a tagged version, of course, I can understand that, as an exception to the rule.
In general I also prefer a tagged release, with the hope/idea being that the project has done some QA and polish before giving it a formal tag.
But the eigen project does not seem to work this way. 2+ years with no release at all.
How much work is it to update eigen in ITK? If small, we could go to tip of the 3.4.1 branch first, digest that, then go to master (which I suspect would be necessary honestly).
I've just asked about the state of master here: https://gitlab.com/libeigen/eigen/-/issues/2699
Great question upstream @seanm. Let's see their answer! Eigen is header-only, and we only pull the core subset in our third-party updates.
I prefer a tagged version, but given the circumstances I ignore the benefit of going towards 3.4.1 over the current tip (besides concerns about the leap to the current tip being too much): we have no guarantee that 3.4.1 will be made into a tag.
Given that the upstream ticket I commented on is 3 months old, has 16 thumbs up, but no actual answers in 3 months, I think we'll just have to make our own decision.
So assuming we prefer a tagged version,
- Staying on the current tip may be a little bit too much, since if a version is tagged and released (e.g. 3.4.1), it may not be at the current tip, and downgrading may be an issue (?).
- If we know which commits are enough to make the warnings go away, maybe we could choose to stay with the head commit that is just enough to remove the warnings. IIRC, @seanm you did some PRs to address them, so I assume it should not be hard to find such commits.
Checking last commits of Eigen, most of them address minor warnings, for different compilers or different architectures. Or refactor code. They don't seem to introduce breaking changes. Our CI checks pass on current master #4265.
I think their master is not the type of breaking changes, but more of a flow of continuous minor fixes, with big bumps when a new hardware architecture, or bridging to new types. We don't use those internally, and don't get affected if there are bugs on those.
Their master requires c++14, but that's something that ITK already requires.
I agree that the optimal would be to stay on a well maintained release, where upstream backports from master to their latest release, but that's not happening in Eigen in a fast way. It's a bit cumbersome having to maintain those in our fork.
Staying on the current tip may be a little bit too much, since if a version is tagged and released (e.g. 3.4.1), it may not be at the current tip, and downgrading may be an issue (?).
If we are ok in our commit in master, we don't need to go back. I hope the release would be ahead of the sha we have frozen. But in any case, we can always downgrade (it's a bit more complicated, but doable).
If we know which commits are enough to make the warnings go away, maybe we could choose to stay with the head commit that is just enough to remove the warnings. IIRC, @seanm you did some PRs to address them, so I assume it should not be hard to find such commits.
Sure, but then, why don't go the tip? They have CI/CD, and if they see something broken, they will fix it, and we will follow. I think the HEAD of master is better than master~100.
If you can't close this yourself @seanm, then @jhlegarreta and @N-Dekker can, and possibly @phcerdan too.
Looks like I could close it myself, but not sure of the etiquette. Generally I defer to the creator of the ticket to decide if it's fixed or not...
OK, my bad. Should have closed it when the PR was merged. Sorry. Thanks for having kept an eye Sean.