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. #119

Merged
merged 4 commits 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. (Same change as just submitted for OpenEXR, see AcademySoftwareFoundation/openexr#961)

Signed-off-by: Matthäus G. Chajdas [email protected]

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 <[email protected]>
Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

Same question as on the corresponding OpenEXR PR - do we need to also bump CMake minimum to 3.13?

@lgritz
Copy link
Contributor

lgritz commented Mar 13, 2021

@meshula Certainly not for 3.0! We should not be changing dependency requirements at this stage.

Let's get this release done, and then as a separate matter, I think the whole TAC should decide if we want to make a policy decision across all projects about what is a reasonable minimum CMake. For a number of reasons, I would support bumping from 3.12 to something newer, but let's do it by consensus and with a principled reason for which version specifically. There are also some really nice things in 3.15, such as the -B an -S arguments to cmake that eliminate the confusing directory dance.

EDIT: -B and -S are in 3.13. But I still think this is a TAC topic.

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 approve the changes, but I feel less strongly that it should be merged before the release.

@cary-ilm
Copy link
Member

This seems innocuous enough to include, and easy enough to back out of if it causes a problem. I vote for merging, then address the cmake version issues later.

@cary-ilm cary-ilm merged commit c106aef 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!

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

Successfully merging this pull request may close these issues.

4 participants