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

CadQuery phase 2: OCP #1099

Closed
wants to merge 1 commit into from
Closed

CadQuery phase 2: OCP #1099

wants to merge 1 commit into from

Conversation

leycec
Copy link
Contributor

@leycec leycec commented Jul 20, 2021

This PR continues the ancient viking saga of CadQuery first transcribed in #1097. We boldly package the next major dependency: OCP, Python bindings to the entirety of Open Cascade auto-generated by pywrap. This PR is much smaller and saner than phase 1, mostly because the only things OCP needs are (A) Open Cascade and (B) pywrap, which are all packaged. We now give thanks.

Since Gentoo slots Open Cascade into the prior 7.4.0 line and ongoing 7.5.x line, this PR follows suite by providing two suspiciously similar OCP ebuilds:

  • cadquery-ocp-7.4.0.ebuild, the most recent stable release of OCP targetting Open Cascade 7.4.0. Shockingly, it works. I know, right? 😮
  • cadquery-ocp-7.5.2_rc20210706.ebuild, the most recent Git commit of OCP targetting Open Cascade 7.5.2. Shockingly, it also works. What is even going on here?

These two ebuilds are mostly duplicates of one other. Minor differences between the two all stem from minor differences between Open Cascade 7.4.0 and 7.5.2. Basically, Open Cascade 7.5.2 supports JSON and is packaged by Gentoo into saner system directories (e.g., /usr/include/opencascade-7.5.2/ rather than /usr/lib64/opencascade-7.4.0/ros/include/). That's it. We now give thanks.

Everything mostly makes sense on the packaging front... subject to these familiar caveats:

  • No tests. Everyone saw that one coming. We couldn't do anything about this with pywrap, but we can do something about this with OCP. All OCP does is provide a single Python C extension! So, we just test that we can import a few well-known Open Cascade attributes from that extension. This increases the likelihood that OCP actually did what it said, which is better than the usual NothingBurger™ we've come to expect from CadQuery packaging. I'll take it.
  • Hard-coded paths. Again, sed solves all these problems and more. Thanks, Stallman, for you continue to grace us with modern solutions to age-old problems.
  • Manual cmake invocation. The cmake eclass doesn't play nicely with the distutils-r1 eclass, so I ditched it. Instead, we invoke cmake manually. Unfortunately, ...sigh the build process by which OCP generates bindings isn't quite as automated as one would hope – by which I mean it's totally unautomated. I had to manually reverse engineer this process from their Anaconda-specific build-bindings-job.yml file. To compound matters, this process needs header include directories for Clang, VTK, and Open Cascade – but there isn't an automated way to query for those directories. Neither Clang, VTK, nor Open Cascade install .pc files for inspection by pkg-config. Although Clang, VTK, and Open Cascade do all install .cmake files for inspection by cmake, OCP ignores those files and instead ships its own broken FindOpenCascade.cmake file that completely fails at its only job. So, we (A) manually find header include directories and (B) patch those directories into FindOpenCascade.cmake. It works, but it sucks. I'll be filing an upstream mega-issue detailing all of this suckage once we finish packaging CadQuery in full.

Upstream will pay for their crimes! Until they do, OCP is actually really amazing – ignoring the packaging crisis detailed above. OCP not only provides one-to-one Python bindings for Open Cascade but, when combined with CadQuery, a much higher-level and easier-to-use API for converting curmudgeonly CAD formats birthed in the seventh circle of Hell like STEP, IGES, and DXF into reasonable VTK PolyData objects.

Honestly, I consider STEP, IGES, and DXF unusable without OCP. All hail CadQuery for it makes the insane things sane again.

@Nowa-Ammerlaan
Copy link
Member

* _Manual `cmake` invocation._ The `cmake` eclass doesn't play nicely with the `distutils-r1` eclass, so I ditched it. Instead, we invoke `cmake` manually. Unfortunately, ..._sigh_ the build process by which OCP generates bindings isn't quite as automated as one would hope – by which I mean it's totally unautomated. I had to manually reverse engineer this process from their Anaconda-specific `build-bindings-job.yml` file. To compound matters, this process needs header include directories for Clang, VTK, and Open Cascade – but there isn't an automated way to query for those directories. Neither Clang, VTK, nor Open Cascade install `.pc` files for inspection by `pkg-config`. Although Clang, VTK, and Open Cascade _do_ all install `.cmake` files for inspection by `cmake`, OCP ignores those files and instead ships its own broken `FindOpenCascade.cmake` file **that completely fails at its only job.** So, we **(A)** manually find header include directories and **(B)** patch those directories into `FindOpenCascade.cmake`. It works, but it sucks. I'll be filing an upstream mega-issue detailing all of this suckage once we finish packaging CadQuery in full.

I think we might be able to workaround the cmake mess with BUILD_DIR/CMAKE_IN_SOURCE_BUILD/CMAKE_USE_DIR, if it doesn't work then we'll just do it like it is now.

@waebbl
Copy link
Contributor

waebbl commented Jul 20, 2021

Since Gentoo slots Open Cascade into the prior 7.4.0 line and ongoing 7.5.x line, this PR follows suite by providing two suspiciously similar OCP ebuilds:

* `cadquery-ocp-7.4.0.ebuild`, the most recent stable release of OCP targetting Open Cascade 7.4.0. Shockingly, it works. I know, right? open_mouth

* `cadquery-ocp-7.5.2_rc20210706.ebuild`, the most recent Git commit of OCP targetting Open Cascade 7.5.2. Shockingly, it also works. What is even going on here?

As maintainer of the opencascade package, I'm planning on removing the 7.4.0 line not very far into the future. AFAIK pkgcheck and repoman can't handle cross-repo dependencies, so a removal would silently break the cadquery-ocp-7.4.0.ebuild. I'd suggest, we stick to the v7.5.2 release and just ignore the v7.4.0 release altogether.

* _Manual `cmake` invocation._ The `cmake` eclass doesn't play nicely with the `distutils-r1` eclass, so I ditched it. Instead, we invoke `cmake` manually. Unfortunately, ..._sigh_ the build process by which OCP generates bindings isn't quite as automated as one would hope – by which I mean it's totally unautomated. I had to manually reverse engineer this process from their Anaconda-specific `build-bindings-job.yml` file. To compound matters, this process needs header include directories for Clang, VTK, and Open Cascade – but there isn't an automated way to query for those directories. Neither Clang, VTK, nor Open Cascade install `.pc` files for inspection by `pkg-config`. Although Clang, VTK, and Open Cascade _do_ all install `.cmake` files for inspection by `cmake`, OCP ignores those files and instead ships its own broken `FindOpenCascade.cmake` file **that completely fails at its only job.** So, we **(A)** manually find header include directories and **(B)** patch those directories into `FindOpenCascade.cmake`. It works, but it sucks. I'll be filing an upstream mega-issue detailing all of this suckage once we finish packaging CadQuery in full.

I haven't looked at your code yet, but all three packages, clang, vtk and occt ship with the new config style .cmake files. From my POV there's no need to manually find header include dirs. We can instead use the get_target_property cmake function to ask cmake for where the header files are located. Imagine a construct like

find_package(vtk 9 CONFIG REQUIRED)
if(TARGET VTK::VTK)
  get_target_property(VTK_INCLUDE_DIRS VTK::VTK INTERFACE_INCLUDE_DIRECTORIES)
endif()

Replace vtk with the targets we actually need. It could (and in theory should) be possible to just delete (or ignore) the old style FindOpenCascade.cmake module file.

@leycec
Copy link
Contributor Author

leycec commented Jul 22, 2021

As maintainer of the opencascade package...

Thank you for the blessed work you have done, @waebbl.

I'm planning on removing the 7.4.0 line not very far into the future.

Excellent! This simplifies my life, for which I am also grateful. I'll drop the 7.4.0 ebuild exactly as you say. 👍

I haven't looked at your code yet...

Let us just safely assume that it's ugly, fragile, and dangerous.

We can instead use the get_target_property cmake function to ask cmake for where the header files are located. Imagine a construct like...

EDIT: Ugh! Nothing I wrote below applies. I conveniently forgot to mention that OCP doesn't actually have a top-level CMakeLists.txt file. Moreover, the sub-level OCP/CMakeLists.txt file it does have is only dynamically generated by manually running this command in our ebuild:

		${EPYTHON} -m bindgen \
			--verbose \
			--njobs $(makeopts_jobs) \
			--libclang "${_CLANG_LIB_FILE}" \
			--include "${_CLANG_INCLUDE_DIR}" \
			--include "${_VTK_INCLUDE_DIR}" \
			all ocp.toml || die

Since that command runs both outside of and before any CMake context, there's no relevant CMakeLists.txt file to patch your clever get_target_property CMake calls into. Upstream really needs to solve that by adding a top-level CMakeLists.txt file governing their full build process (including the initial bindgen command above). I'm confident they'll botch it, but that's okay – because at least then there'd be something for us to fix.

This is me sighing into my mechanical Kinesis Advantage keyboard seemingly refurbished from the 70's Star Trek set. 😩 ⌨️

Yes, I can see it now – a working CMakeLists.txt file, the true Shangri-La of every Gentooer who packages risky bleeding-edge software. Your possibly working code snippet is brilliant, because my cmake skills are on par with my COBOL skills. Now I just need to wait several hours for bindgen to regenerate its non-working CMakeLists.txt file, because I foolishly failed to save a copy of the build tree after src_prepare().

@leycec
Copy link
Contributor Author

leycec commented Jul 22, 2021

@AndrewAmmerlaan: Your proposed cmake rework blows my feeble mind. I'd much rather hand all of this off to the cmake eclass, because I don't trust myself enough to get cmake interactions right.

Under optimistic utopian conditions, this should take a day or two to rework. And then there's reality.

@waebbl
Copy link
Contributor

waebbl commented Jul 23, 2021

EDIT: Ugh! Nothing I wrote below applies. I conveniently forgot to mention that OCP doesn't actually have a top-level CMakeLists.txt file. Moreover, the sub-level OCP/CMakeLists.txt file it does have is only dynamically generated by manually running this command in our ebuild:

		${EPYTHON} -m bindgen \
			--verbose \
			--njobs $(makeopts_jobs) \
			--libclang "${_CLANG_LIB_FILE}" \
			--include "${_CLANG_INCLUDE_DIR}" \
			--include "${_VTK_INCLUDE_DIR}" \
			all ocp.toml || die

Since that command runs both outside of and before any CMake context, there's no relevant CMakeLists.txt file to patch your clever get_target_property CMake calls into. Upstream really needs to solve that by adding a top-level CMakeLists.txt file governing their full build process (including the initial bindgen command above). I'm confident they'll botch it, but that's okay – because at least then there'd be something for us to fix.

I never had to work with toml files before, so excuse me, if my suggestions are not usable.

  • I still think, the FindOpenCASCADE.cmake module might not be necessary. They seem to restrict the list of modules to a selection of the available modules, which, at least in theory, if they want to create bindings for the complete package is possibly only a safeguard mechanism until everything works. IMO it won't hurt, if all available modules are scanned and included by cmake, just some additional, not yet needed definitions. So we can eventually strip the file from the additional_files entry in ocp.toml and influence the creation of the CMakeLists.txt file.
  • Is the file templates/CMakeLists.j2 used when creating the CMakeLists.txt files? If so, we could patch this file to control the creation of the cmake file(s)
find_package(OpenCASCADE CONFIG REQUIRED)
find_package(VTK 9.0 CONFIG REQUIRED COMPONENTS ...)

Note the removal of the COMPONENTS option for occt. There's no component named OPENCASCADE defined in the config file, which might give an error if it's defined. If no components are specified, the occt config file looks for all modules which have been installed. We might also need to check whether the components passed for vtk are correct, but IIRC they are.

@Nowa-Ammerlaan
Copy link
Member

EDIT: Ugh! Nothing I wrote below applies. I conveniently forgot to mention that OCP doesn't actually have a top-level CMakeLists.txt file. Moreover, the sub-level OCP/CMakeLists.txt file it does have is only dynamically generated by manually running this command in our ebuild:

		${EPYTHON} -m bindgen \
			--verbose \
			--njobs $(makeopts_jobs) \
			--libclang "${_CLANG_LIB_FILE}" \
			--include "${_CLANG_INCLUDE_DIR}" \
			--include "${_VTK_INCLUDE_DIR}" \
			all ocp.toml || die

Since that command runs both outside of and before any CMake context, there's no relevant CMakeLists.txt file to patch your clever get_target_property CMake calls into. Upstream really needs to solve that by adding a top-level CMakeLists.txt file governing their full build process (including the initial bindgen command above). I'm confident they'll botch it, but that's okay – because at least then there'd be something for us to fix.

If you call CMAKE_USE_DIR="${BUILD_DIR}/OCP" cmake_src_prepare after the bindgen stuff then the patches are only applied after bindgen has generated the CMakeLists.txt file, or am I missing something? (since cmake_src_prepare callseapply_user and friends)

@leycec
Copy link
Contributor Author

leycec commented Jul 24, 2021

That feeling when you double facepalm yourself in the privacy of your own man-cave.

sci-libs/vtk is PYTHON_SINGLE_TARGET. Both OCP and CadQuery require sci-libs/vtk at runtime, so OCP and CadQuery must also be PYTHON_SINGLE_TARGET. At least it's summer in Canada. We still have this much to cling to.

@Nowa-Ammerlaan Nowa-Ammerlaan self-assigned this Jul 26, 2021
@leycec
Copy link
Contributor Author

leycec commented Jul 26, 2021

If you call CMAKE_USE_DIR="${BUILD_DIR}/OCP" cmake_src_prepare after the bindgen stuff then the patches are only applied after bindgen has generated the CMakeLists.txt file, or am I missing something?

Yupper! You are right, because you are @AndrewAmmerlaan.

Patching CMakeLists.txt with @waebbl's noice cmake logic sadly doesn't help. We still have to manually discover these paths before running bindgen (which generates the CMakeLists.txt file to be patched), because we have to pass these paths to bindgen before doing anything else:

	${EPYTHON} -m bindgen \
		--verbose \
		--njobs $(makeopts_jobs) \
		--libclang "${_CLANG_LIB_FILE}" \
		--include "${_CLANG_INCLUDE_DIR}" \
		--include "${_VTK_INCLUDE_DIR}" \
		all ocp.toml || die

This is why OCP needs a top-level CMakeLists.txt file governing the full build process, but that's a bit out-of-scope for distro-specific packaging. The cost of bleeding-edge software is high indeed.

add OCP, a mandatory dependency of CadQuery

Signed-off-by: Cecil Curry <leycec@gmail.com>
@leycec
Copy link
Contributor Author

leycec commented Jul 26, 2021

Okay! We're now cooking with electrons.

Andrew's exhaustive code review has yielded total success. This is no surprise. The only surprise is that I did what Andrew said this weekend rather than just binge-watch Ted Lasso. In short, I've now:

  • Dropped the 7.4.0 line in accordance with @waebbl's helpful reminder that Open Cascade 7.4.0 is on life support and to be axe-murdered retired shortly.
  • Refactored the 7.5.2 line to use the official 7.5.2 beta release in lieu of unofficial git commits that blow up when you go to compile them.
  • Inherited the 7.5.2 line from the cmake eclass in lieu of manually invoking cmake. Sprinkling a liberal dose of CMAKE_USE_DIR="${S}/OCP" cmake_src_{phase} black magic everywhere worked like a charm. This has the extreme advantage of enabling Gentoo's CMake integration, including full CFLAGS, CXXFLAGS, and LDFLAGS support as well as ANSI colors. Yes, ANSI colors. I didn't even know we did that – but we do.
  • Dropped multiple Python target support in lieu of single Python target support, streamlining everything across the ebuild. OCP now requires the same single Python target that VTK requires. Sanity has been restored.
  • Increased disk usage requirements to 1.3GB, after extensive profiling before, during, and after every ebuild phase. Specifically:
    • CLang-based preparation consumes an initial 400MB.
    • Ninja-based compilation adds an eye-opening 900MB.

We're almost done! But we're not quite there. We still need to regenerate Open Cascade symbols with LIEF (Library for Instrumenting Executable Formats) in the src_prepare() phase before running bindgen against those symbols. I have locally working ebuilds for LIEF and its dependencies, but haven't quite finished integrating that work with OCP.

In any case, let's push this before a freak thunderstorm demagnetizes the last good sectors on the ten year-old mechanical HDD hosting my most sensitive data. ⛈️ 💾

@Nowa-Ammerlaan
Copy link
Member

Awesome, thanks!

There was one left over die hanging around in src_install which I removed while merging.

@leycec
Copy link
Contributor Author

leycec commented Jul 29, 2021

Bam. Thanks so much for the insightful code reviews and cleanup! I really appreciate how helpful, constructive, and fun everyone is here. Another CadQuery PR bites the /dev/null.

Up next: "CadQuery phase 2.5: OCP + LIEF," where we improve the OCP ebuild to dump local OCP symbols with LIEF before generating OCP bindings for safety. The tension is palpable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants