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

consolidate project configurations to one place #1473

Merged
merged 5 commits into from
Aug 17, 2023

Conversation

meshula
Copy link
Contributor

@meshula meshula commented Jun 30, 2023

I'm doing a build that is purely OpenEXRCore. We include <IlmThreadConfig.h> and others from various places in the code base. This change consolidates the OpenEXR C++ configuration incudes into openexr_conf.h so that it's possible to elide them in one spot instead of touching many files. This also ensures that the same preprocessor defines are set consistently across all the source files.

Also, this change adds a missing header guard include, and guards a macro that might be set by some c standard libraries, generating a macro redefinition warning.

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.

Looks like you're getting an error compiling src/bin/exrinfo/main.c

src/lib/OpenEXRCore/internal_dwa_compressor.h Outdated Show resolved Hide resolved
@meshula
Copy link
Contributor Author

meshula commented Jul 12, 2023

Ah, thanks for noticing the tools, I have to add a couple more tweaks to this PR.

Yes, the bounds check fix accidentally crept in to the PR. If I rebase on latest that should resolve it.

Signed-off-by: Nick Porcino <meshula@hotmail.com>
@meshula
Copy link
Contributor Author

meshula commented Aug 14, 2023

I removed the spurious compression fix. Now looking into why ImathConfig.h is not available to the tools when they are built

@cary-ilm
Copy link
Member

@meshula, while you're looking at configs, could you take a look at #1441? It's not clear there's a problem, but it's certainly quite confusing. If there's anything we could do to clean that up, that would be nice.

Signed-off-by: Nick Porcino <meshula@hotmail.com>
@meshula
Copy link
Contributor Author

meshula commented Aug 15, 2023

@cary-ilm it would probably be clearer to simplify our cmake system again to use vanilla cmake instructions rather than an arguably ergonomic macro. I don't know that the result would be improved though, in terms of people seeing the dependencies that make sense. If it were a supported case that it's possible to build Iex without having to also build IlmThread without having to build OpenEXR, it might take extensive finagling to really enable that.

Signed-off-by: Nick Porcino <meshula@hotmail.com>
@meshula meshula force-pushed the config branch 3 times, most recently from f0cca7f to 1a69aed Compare August 15, 2023 04:05
Signed-off-by: Nick Porcino <meshula@hotmail.com>
Signed-off-by: Nick Porcino <meshula@hotmail.com>
@meshula
Copy link
Contributor Author

meshula commented Aug 17, 2023

@cary-ilm I think this is good to go ~ I can't access the "1 change requested", hopefully it's something resolved during a previous push.

@cary-ilm
Copy link
Member

Still not exactly what I had in mind, but it works for now at the very least. I'll submit an alternative shortly for review, I may be missing something. Thanks!

@cary-ilm cary-ilm merged commit 15aa9a7 into AcademySoftwareFoundation:main Aug 17, 2023
29 checks passed
@cary-ilm
Copy link
Member

@meshula, #1523 is what I was trying to suggest. Does that work, or am I missing something?

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

Successfully merging this pull request may close these issues.

2 participants