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

Update build system #115

Merged
merged 6 commits into from
Jan 12, 2023
Merged

Update build system #115

merged 6 commits into from
Jan 12, 2023

Conversation

waebbl
Copy link
Contributor

@waebbl waebbl commented Jan 7, 2023

The patches pick up some ideas from issue #100

  • Consequently use the variables provided by GNUInstallDirs
  • Simplify finding OpenEXR and IlmBase / Imath. OpenEXR looks for either IlmBase (<OpenEXR-3) or Imath (>=OpenEXR-3), so we don't find these dependencies explicitly and only search for OpenEXR with a version provided
  • Testing is enabled globally (enable_testing), but we have a CMAKE_BUILD_TESTS variable. Guard the call of this function by the cmake option.

Bug: #100
Signed-off-by: Bernd Waibel [email protected]

@michaeldsmith
Copy link
Collaborator

@waebbl thanks for the contribution, this looks really good and I like the simpler process. The only issue is it looks like the CI is failing to build under MacOs

image

The valgrind and Windows-vcpkg-test CIs are failing due to issue #109 that I'm working on

@michaeldsmith
Copy link
Collaborator

michaeldsmith commented Jan 8, 2023

It looks like the compiler can't find half when building with OpenEXR 2 under MacOs

image

@waebbl
Copy link
Contributor Author

waebbl commented Jan 9, 2023

Is it possible that it's not recognizing the noexcept keyword? Would it help to pass a certain C++ standard to use?

@michaeldsmith michaeldsmith self-requested a review January 10, 2023 02:03
@michaeldsmith
Copy link
Collaborator

@waebbl your suggestion worked, requiring C++ 11 fixed the MacOs OpenEXR 2 build issue.

@michaeldsmith
Copy link
Collaborator

@waebbl do you think this PR also addresses the GNUInstallDirs issue mentioned in PR #67 ?

@waebbl
Copy link
Contributor Author

waebbl commented Jan 10, 2023

@waebbl do you think this PR also addresses the GNUInstallDirs issue mentioned in PR #67 ?

Yes it should, at least mostly. Some of that PR is outdated, some is fixed with my PR in conjunction with your changes to use GNUInstallDirs.
What my PR doesn't address yet, is the configuration and installation of pkg-config and cmake config files, which is currently not done by the build system. PR #67 includes some patches for the pkg-config files as well, but I'm not sure if the changes are still up-to-date.
That patch was from another Gentoo fellow dev and we used it for a long time, but with the latest patch, supporting OpenEXR-3 / Imath, I no longer need that patch.

@michaeldsmith
Copy link
Collaborator

one more build system question @waebbl, do you think we still need the files in \CTL\config directory and the following lines in \OpenEXR_CTL\CMakeLists.txt ?

if ( PKG_CONFIG_FOUND )
    configure_file(../config/OpenEXR_CTL.pc.in "${PROJECT_BINARY_DIR}/OpenEXR_CTL.pc" @ONLY)
    install( FILES "${PROJECT_BINARY_DIR}/OpenEXR_CTL.pc" DESTINATION lib/pkgconfig COMPONENT dev )
endif()

@waebbl
Copy link
Contributor Author

waebbl commented Jan 11, 2023

It depends.
Many packages which provide new cmake config files, still provide their old pkg-config files as well. So it's up to you, if you still want to distribute the pkg-config files together with cmake config files, we should keep the portion from the CMakeLists.txt file you quoted. But those lines should be prepended by a find_package call for pkg-config.

For the files in CTL/config. If you want to install the pkg-config files, you als need the *.pc.in files. The *.cmake.in files, however, are not needed IMO, as the cmake config files should preferably be created using cmake functions from CMakeConfigPackageHelpers.

@waebbl
Copy link
Contributor Author

waebbl commented Jan 11, 2023

How could I preserve your additions and still edit the files I've changed, i.e. to clean up. Can I just pull your changes into my local branch?

@michaeldsmith
Copy link
Collaborator

@waebbl I pushed my additions to your forked repo waebbl/CTL update-build branch https://github.com/waebbl/CTL/tree/update-build so I think you can just do git pull on your update-build branch https://github.com/waebbl/CTL/tree/update-build

@michaeldsmith
Copy link
Collaborator

@waebbl do you use the pkg-config files in your gentoo package? If not, I'd rather simplify and remove the pkg-config stuff.

@waebbl
Copy link
Contributor Author

waebbl commented Jan 11, 2023

@waebbl I pushed my additions to your forked repo waebbl/CTL update-build branch https://github.com/waebbl/CTL/tree/update-build so I think you can just do git pull on your update-build branch https://github.com/waebbl/CTL/tree/update-build

Thanks, I could just update it in the browser interface.

@waebbl do you use the pkg-config files in your gentoo package? If not, I'd rather simplify and remove the pkg-config stuff.

We currently don't have consumers of the ctl package, so we don't need the pkg-config files. I'm ok with dropping those.

@michaeldsmith
Copy link
Collaborator

@waebbl i pushed a commit to your update-build branch that removes the \CTL\config directory and removes the following code from \OpenEXR_CTL\CMakeLists.txt

if ( PKG_CONFIG_FOUND )
    configure_file(../config/OpenEXR_CTL.pc.in "${PROJECT_BINARY_DIR}/OpenEXR_CTL.pc" @ONLY)
    install( FILES "${PROJECT_BINARY_DIR}/OpenEXR_CTL.pc" DESTINATION lib/pkgconfig COMPONENT dev )
endif()

If you don't see any issues with this, let me know and I'll merge the PR

waebbl and others added 6 commits January 12, 2023 07:43
Use GNUInstallDirs variable throughout the build system.

Bug: ampas#100
Signed-off-by: Bernd Waibel <[email protected]>
The search for OpenEXR and IlmBase resp. Imath dependencies can be
simplified, as OpenEXR depends on either Imath or IlmBase, depending
if we use <OpenEXR-3 or >=OpenEXR-3.

Signed-off-by: Bernd Waibel <[email protected]>
The function enable_testing was globally enabled, no matter the value
of the CMAKE_BUILD_TESTS option. This patch guards this call, so it
only gets called, when CMAKE_BUILD_TESTS=ON.

Signed-off-by: Bernd Waibel <[email protected]>
@waebbl
Copy link
Contributor Author

waebbl commented Jan 12, 2023

@michaeldsmith I cleaned up the code somewhat and actually removed the previously only commented lines of code.
In don't see any issues removing the directory and the pkg-config related code from the CMakeLists.txt file.

@michaeldsmith michaeldsmith merged commit f2474a0 into ampas:master Jan 12, 2023
@waebbl waebbl deleted the update-build branch January 12, 2023 17:06
@waebbl
Copy link
Contributor Author

waebbl commented Jan 12, 2023

Thanks!

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.

2 participants