vcpkg icon indicating copy to clipboard operation
vcpkg copied to clipboard

[openexr3] Add new port

Open simogasp opened this issue 2 years ago • 30 comments

Following the discussion in #20957 with @JackBoosY @chausner this PR introduces the openexr port for its version 3 as a separate port openexr3. OpenEXR 3 introduced many changes in the way it is built, from adopting modern CMake to splitting out a library, Imath, for which a proper port already exists. Some ports still rely on 2.x versions of openexr and they cannot be upgraded without introducing a lot of patchworks or changing the source code. On the other hand, other ports would upgrade to version 3.x and they are blocked

The rationale behind this PR is that like, e.g. opencv, is to separate the two incompatible versions and manage the two separately until version 2.x will be phased out at some point or all the ports will have upgraded their source code to version 3.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

all triplets seem to work, the baseline has been updated

Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

simogasp avatar Jun 14 '22 20:06 simogasp

This new approach makes sense to me, it'll unblock things nicely. Hopefully there's some way that projects depending on OpenEXR 3 (both within and outside of the vcpkg ecosystem) can discover openexr3 without needing to patch their own cmakefiles. If there's something that can be done on the OpenEXR side of the equation to make things easier, please feel free to suggest it.

meshula avatar Jun 15 '22 17:06 meshula

The CMake config problem might be resolved. But the more serious problem is /include/OpenEXR, used by both ports. This will break vcpkg CI as soon as there are consumers for each openexr port.

dg0yt avatar Jun 15 '22 17:06 dg0yt

I underestimated the complexity of having both versions available at the same time. I don't think it is possible to have both versions available at the same time, so if one is installed the other should be not. This is how e.g. opencv works for the different versions opencv2, opencv3 and opencv4. At this point the simplest solution would be to add in both portfiles something like this

if (EXISTS "${CURRENT_INSTALLED_DIR}/share/openexr")
  message(FATAL_ERROR "OpenEXR 2 is installed, please uninstall and try again:\n    vcpkg remove openexr")
endif()

and, mutatis mutandis, the same in openexr 2.

There are not so many ports that depend on openexr (although a big one, opencv), so hopefully they will soon manage to upgrade to openexr 3.

What do you guys think?

simogasp avatar Jun 15 '22 19:06 simogasp

I vote for just going with the fatal error.

In the wild, openexr 2 and 3 do coexist, but never in the same root ~ people often push one of them down, e.g. /my/local/include/openexr2/openexr, and add /my/local/include/openexr2 as needed as an additional system dir. I'm not proposing any action here, just making a note. The fatal error on installing both seems reasonable.

meshula avatar Jun 16 '22 04:06 meshula

The fatal error on installing both seems reasonable.

This is used only as an execption in vcpkg. It excludes one version from CI, and every other port depending on the excluded version. And it may confuse users who pull both versions via transitive dependencies.

/my/local/include/openexr2

This could be a workaround if openexr is upgraded and openexr2 is added as a separate port to satisfy a few remaining consumers, in particular if they use cmake config.

dg0yt avatar Jun 16 '22 04:06 dg0yt

I think it would be better to add judgment to both portfile.cmake. After that we also need to add all triplets of openexr3 as skip to ci.baseline.txt.

Cheney-W avatar Jun 16 '22 06:06 Cheney-W

After that we also need to add all triplets of openexr3 as skip to ci.baseline.txt.

NO. We certainly don't want to skip the new version.

dg0yt avatar Jun 16 '22 06:06 dg0yt

Do you mean skip openexr or make both ports work at the same time?

Cheney-W avatar Jun 16 '22 07:06 Cheney-W

Do you mean skip openexr or make both ports work at the same time?

Skip openexr, the old port. Which has nearly the same effect as simply upgrading openexr, and add the consumers which can't be upgraded to the fail list. Unless they can opt out of the openexr dependency.

dg0yt avatar Jun 16 '22 07:06 dg0yt

@simogasp Could you please modify this PR with following suggestion:

  • [ ] Add judgment in both portfile.cmake of openexr and openexr3
  • [ ] Add all triplets of openexr as skip in ci.baseline.txt

Cheney-W avatar Jun 16 '22 08:06 Cheney-W

@Cheney-W I'll do that. Just to be sure thought, shall I do something like this in the ci.baseline.txt

openexr:arm64-windows      = skip
openexr:arm-uwp            = skip
openexr:x64-linux          = skip
openexr:x64-osx            = skip
openexr:x64-uwp            = skip
openexr:x64-windows        = skip
openexr:x64-windows-static = skip
openexr:x64-windows-static-md=skip
openexr:x86-windows        = skip

and keep the current fail cases of openexr even for openexr3

openexr3:arm64-windows=fail
openexr3:arm-uwp=fail
openexr3:x64-uwp=fail

Is it so? Thanks!

simogasp avatar Jun 16 '22 09:06 simogasp

Yes

Cheney-W avatar Jun 16 '22 10:06 Cheney-W

the problem is that all the ports depending on openexr (most notably opencv) will now fail on CI. Shall I revert back and let openexr build on CI and skip openexr3 (no one depends on this atm)?

PS is there a way with vcpkg command line to list all the packages that depend on a given package? With vcpkg search not all the results pop up, e.g. openimageio depends on openexr but it is not shown, it seems it only checks package names and the optional features (as in opencv3[openexr])

simogasp avatar Jun 16 '22 10:06 simogasp

@simogasp The reason no one depends on openexr3 is because the projects that need to move to openexr3 are blocked due to the unavailability of openexr3 on vcpkg.

Everyone depending on vcpkg supporting openexr3 and no longer being stuck on openexr 2 thanks you for your efforts and patience on this. Please don't skip openexr3!

meshula avatar Jun 16 '22 23:06 meshula

@simogasp Thanks for your PR! If you want this PR to be merged, there are currently two ways:

  1. Add all triplet of openexr3 as skip in the ci.baseline.txt, this can avoid the problem that all ports currently depened on openexr cannot pass the CI test, because if the CI test is not all passed, the PR will not be merged.
  2. Change all the dependencies of ports that depend on openexr to openexr3, and solve any compilation problems that may arise above. If you need to modify upstream code, then you need to get upstream's approval.

Cheney-W avatar Jun 17 '22 07:06 Cheney-W

@simogasp Thanks for your PR! If you want this PR to be merged, there are currently two ways:

  1. Add all triplet of openexr3 as skip in the ci.baseline.txt, this can avoid the problem that all ports currently depened on openexr cannot pass the CI test, because if the CI test is not all passed, the PR will not be merged.
  2. Change all the dependencies of ports that depend on openexr to openexr3, and solve any compilation problems that may arise above. If you need to modify upstream code, then you need to get upstream's approval.

I started 2) with just opencv4 because it's the big one. Still waiting for CI to finish but there is an early error on microsoft.vcpkg.pr (x64_windows) with this message

REGRESSION: vcpkg-ci-opencv:x64-windows cascaded, but it is required to pass. (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

Any clue?

simogasp avatar Jun 17 '22 07:06 simogasp

the problem is that all the ports depending on openexr (most notably opencv) will now fail on CI. Shall I revert back and let openexr build on CI and skip openexr3 (no one depends on this atm)?

That is what I meant when I pointed to the effects of skip : some ports will loose CI testing until upgraded. Note that vcpkg-ci-opencv fails because CI is instructed to that this port must pass now, not just skipped because dependecies are missing. (This is new, and it is a good thing. CC @cenit)

PS is there a way with vcpkg command line to list all the packages that depend on a given package? With vcpkg search not all the results pop up, e.g. openimageio depends on openexr but it is not shown, it seems it only checks package names and the optional features (as in opencv3[openexr])

I'm not aware of such a command. But there is:

./vcpkg depend-info --overlay-ports scripts/test vcpkg-ci-opencv:x64-windows

Studying the dependencies, openexr could be taken out of this special CI port by removing the features openexr and ovis from the dependencies of vcpkg-ci-opencv. At least until these dependency chains are updated or resolved in another way.

But this is just the tip of the iceberg.

$ git grep -l '"openexr"' ports/*/vcpkg.json
ports/devil/vcpkg.json
ports/directxtex/vcpkg.json
ports/freeimage/vcpkg.json
ports/ilmbase/vcpkg.json
ports/magnum-plugins/vcpkg.json
ports/opencv/vcpkg.json
ports/opencv2/vcpkg.json
ports/opencv3/vcpkg.json
ports/opencv4/vcpkg.json
ports/openexr/vcpkg.json
ports/openimageio/vcpkg.json
ports/openvdb/vcpkg.json
ports/osg/vcpkg.json
ports/pangolin/vcpkg.json

These are 13 ports which directly depend on openexr, at least via opt-in features. So CI is potentially disabled for these ports if not upgraded. And you can continue exploring the dependencies.

$ git grep -l '"freeimage"' ports/*/vcpkg.json
ports/colmap/vcpkg.json
ports/forge/vcpkg.json
ports/freeimage/vcpkg.json
ports/ignition-common1/vcpkg.json
ports/ignition-common3/vcpkg.json
ports/ogre-next/vcpkg.json
ports/ogre/vcpkg.json
ports/opencascade/vcpkg.json

...

This is not meant to disccourage your effort. I would rather see it as an encouragement to gain more contributors.

dg0yt avatar Jun 17 '22 07:06 dg0yt

  1. Add all triplet of openexr3 as skip in the ci.baseline.txt, this can avoid the problem that all ports currently depened on openexr cannot pass the CI test, because if the CI test is not all passed, the PR will not be merged.

This would mean to keep the testing of legacy configurations (which are already known to work now) while blocking tests of PRs which migrate openexr consumers to openexr3 (which really need testing). This doesn't make sense to me.

dg0yt avatar Jun 17 '22 08:06 dg0yt

REGRESSION: vcpkg-ci-opencv:x64-windows cascaded, but it is required to pass. (C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt).

Any clue?

Extending my previous comment: You updated opencv4, but there is a transitive dependency via ogre from opencv4[ovis].

dg0yt avatar Jun 17 '22 09:06 dg0yt

what a rabbit hole I put myself in! :laughing:

It seems impossible to update only a bunch of ports to openexr 3 without skipping all the others in the CI. If we have to update all the ports that depend on openexr we are back to @chausner 's work in #20957 and having openexr3 does not make sense anymore, it's better to update the existing one,

This is the original list chausner made to which I added my attempts (OK/NOK notes)

  • [X] devil
  • [x] directxtex --> OK need to fix headers in patch + need a fix in CMakeLists.txt
  • [X] freeimage
  • [ ] magnum-plugins
  • [X] ogre
  • [ ] opencv
  • [ ] opencv2
  • [ ] opencv3
  • [X] opencv4 --> OK
  • [X] openimageio --> OK
  • [X] openvdb --> OK
  • [ ] osg --> [optional] NOK openexr is optional (if the plugins are built) but in the CMakeLists file it searches for it in any case, so it cannot be disabled with the current code. This is clearly a bug in the upstream cmake code
  • [ ] pangolin --> [optional] NOK, it's only optional but it requires some work on the source code to update

simogasp avatar Jun 18 '22 20:06 simogasp

@simogasp Could we convert this PR into draft first?

Cheney-W avatar Jun 22 '22 08:06 Cheney-W

design question: maybe it's better to move current openexr to openexr2, make openexr a metaport that just depends on openexr3 and that's it? so that an overlay just has to replace the dependency of openexr to openexr2

cenit avatar Jun 22 '22 09:06 cenit

design question: maybe it's better to move current openexr to openexr2, make openexr a metaport that just depends on openexr3 and that's it? so that an overlay just has to replace the dependency of openexr to openexr2

In a different way that's the original spirit of this PR. The problem is, even with your proposal, people would like to have openexr 3 in the CI, which is a problem because other ports require openexr 2 to build in CI and we cannot have both versions built at the same time

simogasp avatar Jun 22 '22 20:06 simogasp

if they cannot made be compatible to co-exist, then yes, every port should be made compatible with openexr3 before the upgrade. Same situation for ffmpeg5

cenit avatar Jun 25 '22 10:06 cenit

Alternately, vcpkg should remove the ports that don't consistently update their dependencies. When they update, they ought to be re-added. Due to other less significant projects that are not routinely maintained, OpenEXR version cannot remain stagnant for years in vcpkg; otherwise, vcpkg's relevance will decline.

fabiencastan avatar Jul 04 '22 22:07 fabiencastan

I would like to see this moved forward, integration the patches to consuming ports from https://github.com/microsoft/vcpkg/pull/20957. (I want to update openimageio/opencolorio, #23918.)

if they cannot made be compatible to co-exist, then yes, every port should be made compatible with openexr3 before the upgrade.

I don't think this a viable option in general. It is close to feature development via patches.


This is the original list chausner made to which I added my attempts (OK/NOK notes)

  • [ ] magnum-plugins

Looking into the portfile, it turns out that openexr is only needed for two features, and these features are only built in HEAD mode. So this is not used in vcpkg CI.

  • [ ] opencv

This is actually forwarding to opencv4 which is said to be OK. The dependency is behind an optional feature.

  • [ ] opencv2
  • [ ] opencv3

Not built in CI. The dependency is behind an optional feature.

  • [ ] osg --> [optional] NOK openexr is optional (if the plugins are built) but in the CMakeLists file it searches for it in any case, so it cannot be disabled with the current code. This is clearly a bug in the upstream cmake code
  • [ ] pangolin --> [optional] NOK, it's only optional but it requires some work on the source code to update

If optional, it is not blocking vcpkg CI.

So what is actually lost be moving forward, given that there are manifest mode and overlay ports for users that need an older configuration?

dg0yt avatar Jul 07 '22 04:07 dg0yt

I see that requires:author-response was removed. @simogasp are you willing to continue this PR? The last CI log isn't accessible so it would be good to merge the current master into your branch.

dg0yt avatar Jul 18 '22 05:07 dg0yt

Ping @simogasp for response. Is work still being done for this PR?

Cheney-W avatar Aug 05 '22 07:08 Cheney-W

Ping @simogasp for response. Is work still being done for this PR?

Not at the moment. I'm on vacation and without a proper PC. I'll try to have another go in 20 days or so.

simogasp avatar Aug 05 '22 08:08 simogasp

New direct update PR: #26862.

dg0yt avatar Sep 19 '22 06:09 dg0yt