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

Enable policy 77 if possible. #961

Merged
merged 1 commit into from
Mar 14, 2021

Conversation

Anteru
Copy link
Contributor

@Anteru Anteru commented Mar 13, 2021

When using OpenEXR as a submodule, you currently can't set a variable
outside of it as the option will override it (so you need to set the
option.) With policy 77 in place, this works as expected. See also: https://cmake.org/cmake/help/latest/policy/CMP0077.html

Signed-off-by: Matthäus G. Chajdas dev@anteru.net

When using OpenEXR as a submodule, you currently can't set a variable
outside of it as the option will override it (so you need to set the
option.) With policy 77 in place, this works as expected.

Signed-off-by: Matthäus G. Chajdas <dev@anteru.net>
Copy link
Contributor

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

I was unfamiliar with this policy, https://cmake.org/cmake/help/latest/policy/CMP0077.html
Seems reasonable to me

@lgritz
Copy link
Contributor

lgritz commented Mar 13, 2021

What was the option for which you were having this trouble?

@Anteru
Copy link
Contributor Author

Anteru commented Mar 13, 2021

I was trying to set BUILD_SHARED to OFF externally and that was ignored when using set. When I did set it using option in turn, other sub-projects got confused, so the end result is that I want to control this via a scoped variable. With the policy enabled I seem to be able to correctly build OpenEXR and Imath (that's the other project which needs the same fix) and get static libs without having issues with rebuilds triggering in Visual Studio due to .exp files not found.

@meshula
Copy link
Contributor

meshula commented Mar 13, 2021

I wonder if this also solves the issue I posted where I couldn't specify a CMAKE_INSTALL_PREFIX from the command line and get both OpenEXR and Imath to install there, when Imath is automatically installed by OpenEXR? OpenEXR goes to the right place, but Imath goes to the CMake default location.

@meshula
Copy link
Contributor

meshula commented Mar 13, 2021

77 was introduced in cmake 3.13. Does it make sense to bump our minimum required to 3.13? (If we leave it at 3.12, are people likely to be frustrated and then spend time troubleshooting again, if they have a similar build issue?)

@Anteru
Copy link
Contributor Author

Anteru commented Mar 13, 2021

@meshula I don't know about that, I'm using a local copy of Imath & OpenEXR without trying to pull in Imath via OpenEXR during FETCH_CONTENT. Might be worth a try :)

@Anteru
Copy link
Contributor Author

Anteru commented Mar 13, 2021

I should mention here I had to add it in both OpenEXR and Imath locally to get it to work, see AcademySoftwareFoundation/Imath#119 -- it you want to bump to CMake 3.13, you really need to do it for both projects simultanously.

@lgritz
Copy link
Contributor

lgritz commented Mar 13, 2021

See my comment AcademySoftwareFoundation/Imath#119 (comment)

@meshula
Copy link
Contributor

meshula commented Mar 14, 2021

Progress note: Imath now installs into the correct place, without the policy modification, so that's unrelated to this issue.

@cary-ilm cary-ilm merged commit cf89749 into AcademySoftwareFoundation:master Mar 14, 2021
@Anteru Anteru deleted the enable-policy-77 branch March 14, 2021 07:04
@Anteru
Copy link
Contributor Author

Anteru commented Mar 14, 2021

Thanks!

@cary-ilm cary-ilm added the Build A problem with building or installing the library. label Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build A problem with building or installing the library. v3.0.0-beta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants