-
Notifications
You must be signed in to change notification settings - Fork 626
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
Serious problem: openexr not fully installing #1021
Comments
To be clear: I'm tied up with other things and am NOT currently trying to debug this. |
Confirmed on an M1 Mac. Also, unrelated but notable, the build was bonkers fast compared to my 2020 x86 Mac. (seconds compared to minutes.) |
It fails on Linux as well, and #1008 does seem to be the culprit. Restoring CMakeList.txt and cmake/OpenEXRversion.cmake from pre-1008 fixes the problem, headers get installed as expected. The purpose of #1008 was to consolidate the version settings into the project() statement in the top-level CMakeList.txt, but it clearly has unintended side effects. I'll investigate further, but if someone with more CMake knowledge could look as well, that would be helpful. |
#1022 should fix it. My bad, I accidentally deleted important lines out of the CMakeLists.txt. We really need to beef up our CI to confirm that the install happens properly. |
@cary-ilm : Does this change maybe need cherry picking into 3.0.2? Noticed in Homebrew/homebrew-core#77485 |
My apologies, that was an oversight. I'll merge it and put out a 3.0.3 asap. |
Oh boy, we really do need our CI to check both the install and the ability of projects to consume the config files that we generate. |
Sounds like a new Issue! |
Perhaps tagging pull requests with which branches they need to be back-ported to would help too? |
That would be a nice use of the labels system. |
I love that tagging idea, though I will note that in this case, the patch that fixed it was written and merged at a time when the patch that broke it had not yet been merged into 3.0.2, so it's not clear that anybody would have known to tag it. The issue was that the break and the fix had to go into any branch as a pair, or not at all. |
BTW, I just confirmed that the current RB-3.0 fixes the issue and has a correct install. |
I just noticed another problem with the setting of the IMATH_VERSION and IMATH_VERSION_STRING, the setting of IMATH_VERSION_PATCH appears to be set incorrectly when Imath is fetched via get. It builds properly but with an incorrect version string. This is the cmake output I'm getting, @lgritz, can you confirm this is what you're seeing as well? -- Imath was not found, installing from https://github.com/AcademySoftwareFoundation/Imath.git (v3.0.2) Note that it says it's fetching from v3.0.2, which has IMATH_VERSION_PATCH set to 2, but somehow that's getting changed to 3. |
yeah, interesting. If I build in the way that downloads and incorporates Imath, it gets the openexr version rather than the imath version. Hmmm. It must be some subtlety about how the subprojects work. |
It may have to do with how IMATH_VERSION_* gets set from
CMAKE_PROJECT_VERSION.
On Tue, May 18, 2021 at 1:23 PM Larry Gritz ***@***.***> wrote:
yeah, interesting. If I build in the way that downloads and incorporates
Imath, it gets the openexr version rather than the imath version. Hmmm.
It must be some subtlety about how the subprojects work.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1021 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFC3DGKXFZOK4NHQRUHJGQ3TOLEF3ANCNFSM446OCKGQ>
.
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
|
I would just tag Imath v3.0.3 (even if there is no code change) and call it a day. |
@hjmallon, a new release OpenEXR v3.0.3 should resolve the install issue (as well as the Imath version). I mentioned it on Homebrew/homebrew-core#77485 as well. Thanks for flagging so quickly. |
And regarding labels, I've already been applying labels to PR's to indicate what release they're merged into, but only by hand and after-the-fact,so it's informational but not a reliable preventative measure. I'd really like to have a methodical and comprehensive release checklist, it would help especially when putting out multiple releases close together. |
Oops, there may be a problem with this after all. I believe that the Imath that is tagged 3.0.3 still contains this line in its CMakeLists.txt:
So its ImathConfigVersion.cmake declares the wrong version, and that can cause some serious problems in conjunction with OpenEXR. |
I think that if you build OpenEXR in the way that it also builds Imath, it's all ok? But if you truly build Imath separately, then you have an Imath that declares itself as 3.0.2 and an OpenEXR that thinks 3.0.3, and that can create problems for downstream projects. I didn't notice this in the OIIO CI tests yesterday, because it builds just openexr, relying on it in turn building imath. But when Homebrew upgraded both to 3.0.3 this morning (as separate packages), my own builds on my Mac laptop now have trouble in some circumstances. |
How are you seeing the problem? Everything I did yesterday to validate the
behavior succeeded.
Should we proceed with another patch release of Imath?
…On Wed, May 19, 2021 at 9:52 AM Larry Gritz ***@***.***> wrote:
Oops, there may be a problem with this after all. I believe that the Imath
that is tagged 3.0.3 still contains this line in its CMakeLists.txt:
project(Imath VERSION 3.0.2 LANGUAGES C CXX)
So its ImathConfigVersion.cmake declares the wrong version, and that can
cause some serious problems in conjunction with OpenEXR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1021 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFC3DGOY3SIMPY3SXQZI3TDTOPUEFANCNFSM446OCKGQ>
.
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
|
It's partly my own fault, too. It looks like the confusion is that OIIO's exported OpenImageIOConfig.cmake uses the openexr and imath versions sloppily/interchangeably. It makes a bad assumption that they are in sync. |
I can fix on my end, it's a legit bug on my part. But on the other hand, the declared version really is wrong in Imath. |
I'll patch AcademySoftwareFoundation/Imath#145 into Imath's RV-3.0 branch shortly as a step towards a v3.0.4 Imath release (skipped v3.0.3 since that was just a tag). Let's let it settle for a few days before making the actual release, but I'll get it in place. |
What was happening is that OpenImageIOConfig.cmake was forcing a It wasn't OIIO's build itself that breaks in this case, it's other downstream cmake-based projects that consume OIIO's exported cmake config. The tangled web we weave. Here's the fix on my end: AcademySoftwareFoundation/OpenImageIO#2975 |
This was fixed by #1022 |
I'm seeing builds of OpenEXR (from the current master, and when it automatically builds Imath) fail to fully install. It installs the imath part, but the openexr headers and config files are not showing up in the install.
This only recently started happening. I've never seen it happen for v3.0.1, but OIIO CI started failing several days ago (actually, the CI itself didn't fail; the install of master openexr was broken, and my CI fell back to a syste install of OpenEXR 2.3, so it took me a while to discover the problem).
I can reproduce it by hand. Some bisecting indicates that it may have been introduced (or at least become symptomatic) specifically with the merge of #1008, but I don't understand why that would be the case, and so I'm not 100% certain.
Below is a transcript of me doing this right now on my Mac, at the commit that corresponds to #1008. (I can't seem to reproduce it with earlier commits.) Look particularly at the lines that begin with
-- Installing:
(for what's missing) as well as thels -R
at the end that shows that it really is missing.This is on Mac, but I've seen it on OIIO CI on Linux. Here's an example:
You can see that the libOpenEXR made it, but not the headers, nor the lib/cmake/OpenEXR area.
Can somebody else please try this all on their own and see if you can reproduce?
I haven't yet tried the top of 2.4 and 2.5, but we better make sure those are not broken before we do any releases.
The text was updated successfully, but these errors were encountered: