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

Bazel build fixes. #1836

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Conversation

asa
Copy link
Contributor

@asa asa commented Sep 21, 2024

These are a few fixes I needed to get the included bazel file to play nice with OpenImageIO.
I believe they are general enough to be included for others.
I squashed the previous PR (#1816 )work into this new branch and pushed it here instead.

@asa asa mentioned this pull request Sep 21, 2024
@asa
Copy link
Contributor Author

asa commented Sep 22, 2024

@Vertexwahn @cary-ilm can I get a review?
Had to make the new pr as the rebase pulled in new commits over the top of the ones I needed to sign. This is a clean, single commit of the same thing.

Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

LGTM, with one question about the change to OpenEXRConfig.h.in

@@ -17,9 +17,9 @@
//

#ifndef INCLUDED_OPENEXR_VERSION_H
#define OPENEXR_VERSION_MAJOR @OpenEXR_VERSION_MAJOR@
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious what prompted this change? It should be benign, as I believe OPENEXR_VERSION_* and OpenEXR_VERSION_* have the same values. Not opposed to it, just wondering why it should change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When migrating to bazel we take and make bazel versions of these macros, and having them all consistently upper case seams cleaner for these kinds of external automation.

@cary-ilm cary-ilm merged commit 8d141c5 into AcademySoftwareFoundation:main Sep 23, 2024
31 checks passed
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