Govern library version by OpenEXRVersion.h #1386
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I like the idea. Would also suggest modifying -> Maybe we can also get rid of some other CMake Variables, such as: |
|
Ah, thanks for spotting the rest of them. I'll take another pass. |
|
Ok, I think this is the one.... Config relies on Version. Minimal changes to cmake scripts. @Vertexwahn Unfortunately I can't eliminate those other variables as part of this. They are part of linux pkg and so number shenanigans, so they're out of the realm of things I'm comfortable messing around with! |
|
@Vertexwahn to get the Bazel build up, I need to make OpenEXRVersion.h visible during the build, or it needs to be installed before the build starts (see the corresponding change to CMakelists.txt). How can I address it in the Bazel script? |
|
Here is the diff: A more Bazel way of doing it would be probable to introduce an own target for Version.h - but that is something that does not exist on the CMake site |
|
I like the idea of a target, although I may defer that to a second pass, depending on how much I'm able to upgrade cmake skills in the time I have available. Thank you very much for the patch! |
kdt3rd
approved these changes
Apr 25, 2023
cmake/CMakeLists.txt
Outdated
| @@ -71,11 +71,13 @@ endif() | |||
|
|
|||
| configure_file(OpenEXRConfig.h.in ${CMAKE_CURRENT_BINARY_DIR}/OpenEXRConfig.h) | |||
| configure_file(OpenEXRConfigInternal.h.in ${CMAKE_CURRENT_BINARY_DIR}/OpenEXRConfigInternal.h) | |||
| configure_file(../src/lib/OpenEXRCore/OpenEXRVersion.h ${CMAKE_CURRENT_BINARY_DIR}/OpenEXRVersion.h COPYONLY) | |||
There was a problem hiding this comment.
this should not be needed (see below)
There was a problem hiding this comment.
This one is funky ~ the reason it's there is because OpenEXRConfig.h needs to include <OpenEXRVersion.h> and the file isn't visible to Config, in the ${CMAKE_CURRENT_BINARY_DIR} prior to installation, and so catch-22 style, the library can't be built without this copy. So the COPYONLY thing is just to get a copy of the file into the binary dir at build time.
There was a problem hiding this comment.
Our usual "qualify the path" pattern doesn't work, since config is in binary_dir where the qualified paths don't work.
cmake/CMakeLists.txt
Outdated
|
|
||
| if(OPENEXR_INSTALL) | ||
| install( | ||
| FILES | ||
| ${CMAKE_CURRENT_BINARY_DIR}/OpenEXRConfig.h | ||
| ${CMAKE_CURRENT_BINARY_DIR}/OpenEXRVersion.h |
There was a problem hiding this comment.
couldn't this just be ${CMAKE_PROJECT_SOURCE_DIR}/src/lib/OpenEXRCore/OpenEXRVersion.h?
src/lib/OpenEXRCore/OpenEXRVersion.h
Outdated
| @@ -0,0 +1,11 @@ | |||
| // | |||
There was a problem hiding this comment.
the other include files in OpenEXRCore are snake_case. In my twisted mind, snake_case == C, CamelCase == C++, not sure if we want to keep with that?
There was a problem hiding this comment.
I needed OpenEXRCore to be able to see the file without reaching laterally into another directory which is why it's here; and then I couldn't parse in my mind, is this file "more like" OpenEXRConfig.h, or is it "more like" something that should be here. My first thought had been to put it in "OpenEXR", but "OpenEXR" is higher level than core, so it seemed like opening the door to an unintended dependency cycle (Core needs EXR needs Core).
I'm happy to make it snakecase to tell a cleaner story.
Signed-off-by: Nick Porcino <meshula@hotmail.com>
Signed-off-by: Nick Porcino <meshula@hotmail.com>
Signed-off-by: Nick Porcino <meshula@hotmail.com>
|
@Vertexwahn I saw Kimball's note that after rebasing on his change the Bazel script would fail, I'm not following why that is, or what the fix is. I'm especially confused that it didn't fail on windows, but did on mac/linux. Any help appreciated. |
|
@meshula Just add The diff The |
|
Ah, I see. The rule would be that every file referenced must be exhaustively listed in a Bazel build. |
Signed-off-by: Nick Porcino <meshula@hotmail.com>
Signed-off-by: Nick Porcino <meshula@hotmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a prerequisite for the "single file" version of OpenEXRCore in order that the "single file" version can be used without the invocation of CMake to configure files.
It shifts responsibility for the maintenance of the canonical version number from the CMakeLists.txt file at the root, to macros found in OpenEXRVersion.h. Moving forward, we'd modify the version number in that header file, and the CMake script will automatically pick it up.
This gives a build system agnostic way to manage the versioning of the library.
@Vertexwahn I imagine this would also simplify maintenance of the Bazel build if the Bazel script was modified to parse the version automatically.