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

openexr 3.0.1 #74383

Closed
wants to merge 16 commits into from
Closed

openexr 3.0.1 #74383

wants to merge 16 commits into from

Conversation

chenrui333
Copy link
Member

action-homebrew-bump-formula


Created with brew bump-formula-pr.

resource blocks may require updates.

@chenrui333 chenrui333 added the build failure CI fails while building the software label Apr 2, 2021
@chenrui333 chenrui333 force-pushed the bump-openexr-3.0.1 branch from 600f6a5 to 105f6fd Compare April 3, 2021 15:06
@chenrui333 chenrui333 mentioned this pull request Apr 5, 2021
@chenrui333 chenrui333 force-pushed the bump-openexr-3.0.1 branch from 105f6fd to 7a55dd1 Compare April 5, 2021 04:42
@carlocab
Copy link
Member

We should probably have a separate formula for https://github.com/AcademySoftwareFoundation/Imath, as openexr will vendor this if it's not found.

@hjmallon hjmallon mentioned this pull request Apr 30, 2021
5 tasks
@carlocab carlocab force-pushed the bump-openexr-3.0.1 branch 2 times, most recently from 3c6a94f to e3e4c85 Compare April 30, 2021 12:48
@hjmallon
Copy link
Contributor

hjmallon commented Apr 30, 2021

Looks like:

  • openvdb brew formula needs porting ilmbase -> imath
  • ctl needs cmake patching to remove ilmbase search
  • jpeg-xl needs to either include ImfInt64.h manually or port away from using OpenEXT::Int64 to int64_t etc.

@carlocab
Copy link
Member

  • ctl needs cmake patching to remove ilmbase search
  • jpeg-xl needs to either include ImfInt64.h manually or port away from using OpenEXT::Int64 to int64_t etc.

We should check if these two are already being tracked upstream. If they are, maybe there is already a patch we can use. If not, then it should be reported.

@hjmallon
Copy link
Contributor

@carlocab
Copy link
Member

carlocab commented Apr 30, 2021

  • ctl appears dead? - brew formula already includes patches for the last set of openexr changes

If fixing it to work with imath isn't too complicated then we can try patching it to work but also deprecate it as unsupported. If it's too much work we can just disable it as :does_not_build.

@carlocab
Copy link
Member

ARM:

Error: 3 failed steps!
brew install --verbose --build-bottle ctl
brew install --verbose --build-bottle jpeg-xl
brew install --verbose --build-bottle openvdb

All Intel Nodes:

Error: 9 failed steps!
brew test --retry --verbose caffe
brew test --retry --verbose openalpr
brew linkage --test opencv
brew test --retry --verbose opencv
brew linkage --test opencv@3
brew test --retry --verbose opencv@3
brew install --verbose --build-bottle ctl
brew install --verbose --build-bottle jpeg-xl
brew install --verbose --build-bottle openvdb

@carlocab
Copy link
Member

carlocab commented Apr 30, 2021

caffe and openalpr are from missing libraries; they depend on ilmbase though one of the opencv formulae (which depend on openexr). I suspect these won't work with imath.

@cho-m
Copy link
Member

cho-m commented May 5, 2021

@carlocab
Copy link
Member

carlocab commented May 7, 2021

Looks like this will require some work. I guess we can add an openexr@2 formula in the meantime for the dependent formulae that don't work with the latest openexr yet.

Also, switch dependency `ilmbase` to `imath`. `ilmbase` has been
deprecated and been replaced with `imath`.
@carlocab carlocab force-pushed the bump-openexr-3.0.1 branch 2 times, most recently from ad4b3f8 to 2319a70 Compare May 7, 2021 17:28
carlocab and others added 11 commits May 8, 2021 03:31
A few formulae still don't work with the latest version of `openexr`, so
let's carry an earlier compatible version in the meantime.
This is not yet compatible with the latest version of `openexr`.
This is not yet compatible with the latest version of `openexr`.
This is not yet compatible with the latest version of `openexr`.
`ilmbase` is deprecated and has been replaced with `imath`.
@carlocab
Copy link
Member

carlocab commented May 9, 2021

With ilmbase made keg_only, is the conflicts_with in imath unnecessary?

Yes, I was thinking that. However, I didn't realise till well after I made changes to this PR, and I'd rather not have to re-run it: a) it takes ages, and b) caffe's test is flaky, so we risk having to re-run the tests a few more times with further changes to this PR.

@cho-m
Copy link
Member

cho-m commented May 9, 2021

I think it is fine to merge this, then update imath.

The only thing I wasn't sure about would be how dependency conflicts are handled if someone has a combination of formula installed like:

  • imagemagick (openexr + ilmbase --> openexr + imath), and
  • jpeg-xl (openexr + ilmbase --> openexr@2 + ilmbase)

And tries to upgrade in the interval that imath still has conflicts_with.


Though, getting CI to run a PR on imath by itself hopefully won't take that long.

@carlocab
Copy link
Member

carlocab commented May 9, 2021

Yes. Ideally, we would have rev-bumped ilmbase here so that users who have it installed get it unlinked from their prefix before upgrading openexr.

carlocab added a commit to carlocab/homebrew-core that referenced this pull request May 9, 2021
`ilmbase` will become keg-only with Homebrew#74383, so the `conflicts_with` is
unnecessary.
This was referenced May 9, 2021
@carlocab
Copy link
Member

carlocab commented May 9, 2021

imath: #76889
ilmbase: #76890

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Merging this now to avoid merge conflicts from catching up with this PR.

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

carlocab added a commit that referenced this pull request May 9, 2021
`ilmbase` is now keg-only with #74383, so the `conflicts_with` is
unnecessary.
carlocab added a commit to carlocab/homebrew-core that referenced this pull request May 9, 2021
`ilmbase` will become keg-only with Homebrew#74383. Let's bump the revision to
avoid the issues described in Homebrew#74383 (comment).
BrewTestBot pushed a commit that referenced this pull request May 9, 2021
`ilmbase` will become keg-only with #74383. Let's bump the revision to
avoid the issues described in #74383 (comment).

Closes #76890.

Signed-off-by: BrewTestBot <[email protected]>
@carlocab
Copy link
Member

carlocab commented May 9, 2021

I think it is fine to merge this, then update imath.

The only thing I wasn't sure about would be how dependency conflicts are handled if someone has a combination of formula installed like:

  • imagemagick (openexr + ilmbase --> openexr + imath), and
  • jpeg-xl (openexr + ilmbase --> openexr@2 + ilmbase)

And tries to upgrade in the interval that imath still has conflicts_with.

Though, getting CI to run a PR on imath by itself hopefully won't take that long.

This should be sorted now, I think.

@hjmallon
Copy link
Contributor

hjmallon commented May 10, 2021

I got some errors while brew upgrading

==> Pouring imath--3.0.1.big_sur.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink lib/libImath.dylib
Target /usr/local/lib/libImath.dylib
is a symlink belonging to ilmbase. You can unlink it:
  brew unlink ilmbase

To force the link and overwrite all conflicting files:
  brew link --overwrite imath

To list all files that would be deleted:
  brew link --overwrite --dry-run imath

Possible conflicting files are:
/usr/local/lib/libImath.dylib -> /usr/local/Cellar/ilmbase/2.5.5/lib/libImath.dylib
==> Summary
🍺  /usr/local/Cellar/imath/3.0.1: 49 files, 884.9KB
==> Installing opencv dependency: openexr
==> Pouring openexr--3.0.1.big_sur.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink include/OpenEXR/Iex.h
Target /usr/local/include/OpenEXR/Iex.h
is a symlink belonging to ilmbase. You can unlink it:
  brew unlink ilmbase

To force the link and overwrite all conflicting files:
  brew link --overwrite openexr

To list all files that would be deleted:
  brew link --overwrite --dry-run openexr

Possible conflicting files are:
/usr/local/include/OpenEXR/Iex.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/Iex.h
/usr/local/include/OpenEXR/IexBaseExc.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IexBaseExc.h
/usr/local/include/OpenEXR/IexErrnoExc.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IexErrnoExc.h
/usr/local/include/OpenEXR/IexExport.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IexExport.h
/usr/local/include/OpenEXR/IexForward.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IexForward.h
/usr/local/include/OpenEXR/IexMacros.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IexMacros.h
/usr/local/include/OpenEXR/IexMathExc.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IexMathExc.h
/usr/local/include/OpenEXR/IexMathFloatExc.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IexMathFloatExc.h
/usr/local/include/OpenEXR/IexMathIeeeExc.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IexMathIeeeExc.h
/usr/local/include/OpenEXR/IexNamespace.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IexNamespace.h
/usr/local/include/OpenEXR/IexThrowErrnoExc.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IexThrowErrnoExc.h
/usr/local/include/OpenEXR/IlmThread.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IlmThread.h
/usr/local/include/OpenEXR/IlmThreadExport.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IlmThreadExport.h
/usr/local/include/OpenEXR/IlmThreadForward.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IlmThreadForward.h
/usr/local/include/OpenEXR/IlmThreadMutex.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IlmThreadMutex.h
/usr/local/include/OpenEXR/IlmThreadNamespace.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IlmThreadNamespace.h
/usr/local/include/OpenEXR/IlmThreadPool.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IlmThreadPool.h
/usr/local/include/OpenEXR/IlmThreadSemaphore.h -> /usr/local/Cellar/ilmbase/2.5.5/include/OpenEXR/IlmThreadSemaphore.h
/usr/local/lib/libIex.dylib -> /usr/local/Cellar/ilmbase/2.5.5/lib/libIex.dylib
/usr/local/lib/libIlmThread.dylib -> /usr/local/Cellar/ilmbase/2.5.5/lib/libIlmThread.dylib
==> Summary
🍺  /usr/local/Cellar/openexr/3.0.1: 176 files, 5.3MB

I think brew hasn't noticed that openexr 3.0.1 and imath 3.0.1 require ilmbase to be 2.5.5_1 (if it is installed)

@carlocab
Copy link
Member

carlocab commented May 10, 2021

I think brew hasn't noticed that openexr 3.0.1 and imath 3.0.1 require ilmbase to be 2.5.5_1 (if it is installed)

Yea, I thought ilmbase would be "upgraded" before openexr was, but I was mistaken. Guess my revision bump was pointless. I'd be open to adding caveats to the ilmbase formula that explains what to do when encountering this error. Would you like to open a PR for that?

@hjmallon
Copy link
Contributor

Would you like to open a PR for that?

Something like this #76965?

@carlocab
Copy link
Member

Yup, pretty much what I had in mind. Thanks very much, @hjmallon!

@lgritz
Copy link

lgritz commented May 12, 2021

Hi, I noticed that if I build Imath or OpenEXR 3.0.1 from source, it produces and installs cmake config files (ImathConfig*.cmake, OpenEXRConfig*.cmake) installed in $prefix/lib/cmake/{Imath,OpenEXR}. But no such config files end up in /usr/local/lib with homebrew.

@carlocab
Copy link
Member

Not really sure why that's happening... what commands do you use to build from source? All brew is doing here is calling cmake then make.

@hjmallon
Copy link
Contributor

@lgritz : The CMake configs are symlinking fine for me. Is there something different about your setup to mine? Do you use an Apple Silicon machine and have brew under /opt/ maybe? Do the cmake configs appear in ls /usr/local/Cellar/imath/3.0.1/lib?

% brew unlink imath 
Unlinking /usr/local/Cellar/imath/3.0.1... 7 symlinks removed.

% brew link imath -n
Would link:
/usr/local/include/Imath
/usr/local/lib/cmake/Imath    <-------
/usr/local/lib/libImath-3_0.27.0.0.dylib
/usr/local/lib/libImath-3_0.27.dylib
/usr/local/lib/libImath-3_0.dylib
/usr/local/lib/libImath.dylib
/usr/local/lib/pkgconfig/Imath.pc

@lgritz
Copy link

lgritz commented May 13, 2021

Aha, I see now. Because I still had the old IlmBase, it never did the "brew link" step for imath (because of conflicting files).
After "brew unlink ilmbase ; brew link imath", the cmake files are where I expect.

So it's important to brew unlink ilmbase before installing imath, I think.

@carlocab
Copy link
Member

Hm, yes. Perhaps it was in fact better to keep conflicts_with rather than make ilmbase keg-only. Whoops. Hopefully the caveats mitigate that a bit:

==> Caveats
`ilmbase` has been replaced by `imath`. You may want to `brew uninstall ilmbase`
or `brew unlink ilmbase` to prevent conflicts.

@chenrui333 chenrui333 deleted the bump-openexr-3.0.1 branch December 18, 2022 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build failure CI fails while building the software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants