opencv: add new version, variant, and patch by glennpj · Pull Request #27374 · spack/spack

glennpj · 2021-11-11T21:43:59Z

added version 4.5.4
added tesseract variant
added patch to not add system paths

spackbot-app · 2021-11-11T21:44:06Z

@bvanessen can you review this PR?

This PR modifies the following package(s), for which you are listed as a maintainer:

opencv

adamjstewart

Tesseract doesn't seem to be in OpenCV proper like all other components/modules we have variants for, it seems to be in opencv_contrib. Since is the first time we've added a opencv_contrib-specific component, I'll have to think if this is the best way to do this. We'll definitely need a conflict for +tesseract when ~contrib.

var/spack/repos/builtin/packages/opencv/cmake_no-system-paths.patch

var/spack/repos/builtin/packages/opencv/package.py

glennpj · 2021-11-11T23:18:14Z

We'll definitely need a conflict for +tesseract when ~contrib.

Yes, sorry, I completely missed that.

adamjstewart · 2021-11-12T17:19:02Z

Tesseract doesn't seem to be in OpenCV proper like all other components/modules we have variants for, it seems to be in opencv_contrib. Since is the first time we've added a opencv_contrib-specific component, I'll have to think if this is the best way to do this.

@bvanessen do you have any thoughts on this? I'm thinking we might just want to make it a separate variant (not in modules or components).

glennpj · 2021-11-12T19:43:48Z

I just discovered that there seems to be a problem with the installation of opencv that, as far as I can tell, is independent of this PR. Looking at the installation directory, the programs are not getting installed in ${prefix}/bin. This is true for an installation of 4.5.2 that I did back in July as well.

glennpj · 2021-11-12T20:43:33Z

Actually, I think things are fine. I did not realize that literally every variant is turned off.

glennpj · 2021-11-17T18:00:03Z

I converted this to "draft" status as I found some components that do not build due to changed variable names. It looks like this began with version 4.5.2 but it seems like fixing those here is appropriate.

glennpj · 2021-11-23T18:21:38Z

The changes wound being significant but applications and contrib modules are now available as variants, while components that can not be built are no longer variants.

Summary:

- all prebuilt apps are now variants and can be installed
- core is no longer a variant. It was always built anyway so it was not
  really a variant.
- contrib is no longer a variant. All of the contrib modules are now
  available as variants.
- components that can not be built with Spack are no longer variants.
  They are set to 'off' to prevent pulling from system.
- handle the case where a module and a component have the same name
- use `with when` framework
- adjust dependencies and conflicts
- new package: libraw1394
- have libdc1394 depend on libraw1394
- patch to find clp
- patch to find onnx
- patch for cvv to find Qt
- format with black

var/spack/repos/builtin/packages/libdc1394/package.py

var/spack/repos/builtin/packages/opencv/package.py

var/spack/repos/builtin/packages/libdc1394/package.py

adamjstewart · 2021-11-29T21:37:27Z

Looks like there are some conflicts, can you resolve those?

traversaro

The libdc1394 changes look ok for me @adamjstewart .

glennpj · 2021-11-30T15:17:31Z

Looks like there are some conflicts, can you resolve those?

It looks like there were permissions changes on several files in the develop branch that were put in place after my last update of develop that this PR was created from. A rebase seems to have resolved it.

adamjstewart

This PR should contain:

changes to the OpenCV package
changes to other packages that are required as a result (i.e. removing +core)

This PR should not contain changes to other packages that are not related to OpenCV (i.e. fixing a broken build). This makes it harder for me to review. Also, it looks like there was a bad rebase, so this PR is actually undoing many of the changes that have been made in other PRs.

var/spack/repos/builtin/packages/3dtk/package.py

var/spack/repos/builtin/packages/bohrium/package.py

var/spack/repos/builtin/packages/cntk/package.py

var/spack/repos/builtin/packages/libdc1394/package.py

var/spack/repos/builtin/packages/libraw1394/package.py

adamjstewart · 2022-01-27T20:37:43Z

var/spack/repos/builtin/packages/openvslam/package.py

-# SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-
-class Openvslam(CMakePackage):


This package should be deprecated before being removed in case someone is still using it

See #27374 (comment)

By "remove" I mean deprecate first and delete in a later Spack release

Even though there is no code, just a README?

var/spack/repos/builtin/packages/py-astropy/package.py

var/spack/repos/builtin/packages/py-mmcv/package.py

var/spack/repos/builtin/packages/py-torchgeo/package.py

var/spack/repos/builtin/packages/saga-gis/package.py

glennpj · 2022-01-28T18:17:06Z

This PR should contain:

changes to the OpenCV package

changes to other packages that are required as a result (i.e. removing +core)

This PR should not contain changes to other packages that are not related to OpenCV (i.e. fixing a broken build). This makes it harder for me to review. Also, it looks like there was a bad rebase, so this PR is actually undoing many of the changes that have been made in other PRs.

I moved quite a few packages out of this PR into other PRs earlier, as you had requested. I agree that something seemed to have gone horribly wrong with the rebase and I wound undoing much of that. I will have to sort that out, sorry for the extra work.

glennpj · 2022-01-29T20:53:21Z

The problem was not the rebase per se, but a git mistake I made when I moved packages to separate PRs. After creating the other PRs I added commits to this PR to remove the packages. When I rebased, those commits replayed on top and effectively put the packages back in this PR, and reversed the desired commits from the other PRs. I suppose one could argue that doing a rebase was a mistake as I could have merged in the develop branch and resolved conflicts from there. Either way, I did not manage git properly and I apologize for the confusion and extra work.

glennpj · 2022-01-29T21:59:46Z

The 3dtk package needs the new aruco variant in this PR.

glennpj · 2022-01-29T22:01:17Z

The bohrium package needs the new cudev variant.

glennpj · 2022-01-29T22:07:05Z

The py-torchgeo package needs the new datasets variant.

adamjstewart

Testing the build and some packages I care about now

adamjstewart · 2022-01-30T18:25:32Z

var/spack/repos/builtin/packages/opencv/package.py

+    conflicts("+v4l", when="platform=darwin", msg="Linux only")
+    conflicts("+win32ui", when="platform=darwin", msg="Windows only")
+    conflicts("+win32ui", when="platform=linux", msg="Windows only")
+    conflicts("+win32ui", when="platform=cray", msg="Windows only")


We can now make the variant conditional on the OS, although that might be difficult to do with the for-loop.

var/spack/repos/builtin/packages/py-torchgeo/package.py

The py-torchgeo package does not need opencv+datasets.

zip --> zlib

adamjstewart

I haven't looked through the changes in extreme detail (like the conflicts stuff) but I tested that the package installs and works with the only project I use it for (TorchGeo). I'm fine with merging this in its current state unless there are any remaining tests or reviewers you would like to wait for.

glennpj · 2022-01-31T22:52:54Z

I have not put openvslam back in yet. I am not really sure what it means to have a package that has no code, even deprecated packages should be buildable, I think. Otherwise, I think this is ready to go.

adamjstewart · 2022-02-01T00:04:20Z

Yeah, it's mostly for people who already have the package in their source cache. But I'm not even sure we use the source cache when downloading from master anway.

glennpj · 2022-02-01T01:53:09Z

This is a bit off topic but I looked into openvslam a little more. The only version in spack is master and that is when I discovered that there was only a README. However, there are releases that could be pulled. Interestingly, the README has been updated recently with a link to a lengthy discussion about legal issues and academic misconduct. I did not read the whole issue but it makes me think more firmly that we should remove it from spack.

https://github.com/OpenVSLAM-Community/openvslam/blob/in-discussion/README.md

So, I say let's merge this PR. We can deal with openvslam later if desired.

* opencv: add new version, variant, and patch - added version 4.5.4 - added tesseract variant - added patch to not add system paths * Add leptonica depends and contrib conflicts * Add dependencies for 1394 support - new package: libraw1394 - add sdl dependency to libdc1394 - add conflict for openjpeg and jasper * Adjust dependencies and conflicts for opencv modules * rewrite of opencv - all prebuilt apps are now variants and can be installed - core is no longer a variant. It was always built anyway so it was not really a variant. - contrib is no longer a variant. All of the contrib modules are now available as variants. - components that can not be built with Spack are no longer variants. They are set to 'off' to prevent pulling from system. - handle the case where a module and a component have the same name - use `with when` framework - adjust dependencies and conflicts - new package: libraw1394 - have libdc1394 depend on libraw1394 - patch to find clp - patch to find onnx - patch for cvv to find Qt - format with black * Incorporate recommended changes - fix variants and dependencies on packages that depend on opencv - remove opencv-3.2 and patches - add some new patches to handle different versions - cntk needs further work - the openvslam package was markde deprecated as it is no longer an active project and the repository has no code * Remove gmake dependency. * Remove sdl support SDL is only used in an example case, but the examples are not built. * remove openvslam * Remove opencv+flann variant from 3dtk * Back out cfitsio constraint from py-astropy * remove opencv+flann variant from dlib * remove boost constraint from 3dtk * Remove non-opencv related bohrium changes * Adjustments for cntk - protobuf constraint at version 3.10 - need specific variants for opencv - improve patch * Deprecate CNTK package * variant tweaks - added appropriate conflicts for cublas - made cuda/cudev relationship explicit - moved openx to pending components as it needs an openvx package * fix isort style error * Use date version from kaldi rather than commit * Revert changes from a bad rebase * Add +flann to 3dtk and dlib * Use compression support with libtiff * remove `+datasets` from opencv dependency The py-torchgeo package does not need opencv+datasets. * fix typo zip --> zlib

svenevs · 2022-02-25T03:23:13Z

Nice work @glennpj 🙂 I'm not in a position to update my spack right now so I just bludgeoned in sfm after realizing I didn't get it with my install a few months ago. Took a quick look to see if current develop had anything related to this module, you totally nailed it! This package is looking really great now ❤️ Looking forward to the next time I get to re-tool things.

spackbot-app bot added dependencies new-version patch update-package labels Nov 11, 2021

spackbot-app bot requested a review from adamjstewart November 11, 2021 21:44

adamjstewart reviewed Nov 11, 2021

View reviewed changes

var/spack/repos/builtin/packages/opencv/cmake_no-system-paths.patch Outdated Show resolved Hide resolved

var/spack/repos/builtin/packages/opencv/package.py Outdated Show resolved Hide resolved

glennpj force-pushed the opencv branch from d232d62 to 945f012 Compare November 11, 2021 23:38

spackbot-app bot added the conflicts label Nov 11, 2021

tldahlgren assigned adamjstewart Nov 16, 2021

glennpj marked this pull request as draft November 17, 2021 17:58

spackbot-app bot added extends maintainers new-package new-variant resources labels Nov 23, 2021

glennpj marked this pull request as ready for review November 23, 2021 18:17

adamjstewart reviewed Nov 23, 2021

View reviewed changes

spackbot-app bot added the python label Nov 29, 2021

traversaro reviewed Nov 29, 2021

View reviewed changes

var/spack/repos/builtin/packages/libdc1394/package.py Outdated Show resolved Hide resolved

traversaro reviewed Nov 30, 2021

View reviewed changes

glennpj force-pushed the opencv branch from b685617 to 9fe9f47 Compare November 30, 2021 15:11

Use date version from kaldi rather than commit

cc21a8c

glennpj requested a review from adamjstewart January 25, 2022 16:14

adamjstewart reviewed Jan 27, 2022

View reviewed changes

glennpj marked this pull request as draft January 28, 2022 18:17

Revert changes from a bad rebase

a56eae1

glennpj added 2 commits January 29, 2022 16:07

Add +flann to 3dtk and dlib

280ae29

Merge branch 'develop' into opencv

e13135d

glennpj mentioned this pull request Jan 29, 2022

libdc1394: update dependencies and add libraw1394 dependency #28675

Merged

glennpj marked this pull request as ready for review January 29, 2022 22:47

glennpj requested a review from adamjstewart January 29, 2022 22:47

adamjstewart reviewed Jan 30, 2022

View reviewed changes

var/spack/repos/builtin/packages/py-torchgeo/package.py Outdated Show resolved Hide resolved

glennpj added 3 commits January 30, 2022 18:10

Use compression support with libtiff

6a7c2d3

remove +datasets from opencv dependency

9f461c0

The py-torchgeo package does not need opencv+datasets.

fix typo

c53a231

zip --> zlib

adamjstewart approved these changes Jan 31, 2022

View reviewed changes

adamjstewart merged commit 21524f5 into spack:develop Feb 1, 2022

glennpj deleted the opencv branch February 1, 2022 23:25

		# SPDX-License-Identifier: (Apache-2.0 OR MIT)


		class Openvslam(CMakePackage):

Conversation

glennpj commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spackbot-app bot commented Nov 11, 2021

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glennpj commented Nov 11, 2021

Uh oh!

adamjstewart commented Nov 12, 2021

Uh oh!

glennpj commented Nov 12, 2021

Uh oh!

glennpj commented Nov 12, 2021

Uh oh!

glennpj commented Nov 17, 2021

Uh oh!

glennpj commented Nov 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamjstewart commented Nov 29, 2021

Uh oh!

traversaro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennpj commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamjstewart Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

glennpj Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

adamjstewart Jan 30, 2022

Choose a reason for hiding this comment

Uh oh!

glennpj Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glennpj commented Jan 28, 2022

Uh oh!

glennpj commented Jan 29, 2022

Uh oh!

glennpj commented Jan 29, 2022

Uh oh!

glennpj commented Jan 29, 2022

Uh oh!

glennpj commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

adamjstewart Jan 30, 2022

Choose a reason for hiding this comment

glennpj commented Nov 11, 2021 •

edited

Loading

traversaro left a comment •

edited

Loading

glennpj commented Nov 30, 2021 •

edited

Loading

glennpj commented Jan 29, 2022 •

edited

Loading

glennpj commented Jan 31, 2022 •

edited

Loading