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

Remove OpenColorIOHeaders target and hardcoded install paths #1471

Merged

Conversation

remia
Copy link
Collaborator

@remia remia commented Aug 26, 2021

Following @lgritz comment in #1397, I think we can remove the OpenColorIOHeaders to simplify the CMake scripts a bit. I had some issues with documentation build because I first generated the OpenColorIOABI.h in a new location but resolved this by using the exact same target path as before which I think is the simplest solution.

There shouldn't be a need to merge this for the upcoming release.

remia and others added 2 commits August 26, 2021 11:49
hodoulp and others added 2 commits August 31, 2021 09:51
@remia
Copy link
Collaborator Author

remia commented Sep 1, 2021

Updated install paths to use CMake automatic variables as suggested in #1296. Also updated public headers installation to use PUBLIC_HEADER target property. Using these new variables could update the install folder structure on Windows, while probably being more "as expected".

@hodoulp
Copy link
Member

hodoulp commented Sep 1, 2021

@remia Following the last commit, do you need to update the CMakeLists.txt from all the apps, all the libutils, and the Python bindings?

Signed-off-by: Rémi Achard <remiachard@gmail.com>
@hodoulp
Copy link
Member

hodoulp commented Sep 1, 2021

There is also some hard-coded lib to change for the Python bindings i.e. src/bindings/python/CMakeLists.txt at line 229 & 231

@remia
Copy link
Collaborator Author

remia commented Sep 1, 2021

Thanks @hodoulp good point, it seems like we should stop using ${LIB_SUFFIX} according to https://gitlab.kitware.com/cmake/cmake/-/issues/18640#note_488642.

Signed-off-by: Rémi Achard <remiachard@gmail.com>
@remia remia changed the title Remove OpenColorIOHeaders target Remove OpenColorIOHeaders target and hardcoded install paths Sep 1, 2021
@hodoulp
Copy link
Member

hodoulp commented Sep 1, 2021

@michdolan The PR is a candidate for RB-2.1

@hodoulp
Copy link
Member

hodoulp commented Sep 8, 2021

@michdolan or @doug-walker The PR is close to the two-weeks rule, could one of you have a look? Thanks.

@doug-walker doug-walker merged commit 641abe5 into AcademySoftwareFoundation:master Sep 9, 2021
hodoulp added a commit that referenced this pull request Sep 9, 2021
* Remove OpenColorIOHeaders target

Signed-off-by: Rémi Achard <remiachard@gmail.com>

* Avoid hardcoding install paths

Signed-off-by: Rémi Achard <remiachard@gmail.com>

* Update remaining hardcoded install paths

Signed-off-by: Rémi Achard <remiachard@gmail.com>

* Replace LIB_SUFFIX by CMAKE_INSTALL_LIBDIR

Signed-off-by: Rémi Achard <remiachard@gmail.com>

Co-authored-by: Patrick Hodoul <patrick.hodoul@autodesk.com>
hodoulp added a commit that referenced this pull request Sep 23, 2021
…1490)

* Remove OpenColorIOHeaders target

Signed-off-by: Rémi Achard <remiachard@gmail.com>

* Avoid hardcoding install paths

Signed-off-by: Rémi Achard <remiachard@gmail.com>

* Update remaining hardcoded install paths

Signed-off-by: Rémi Achard <remiachard@gmail.com>

* Replace LIB_SUFFIX by CMAKE_INSTALL_LIBDIR

Signed-off-by: Rémi Achard <remiachard@gmail.com>

Co-authored-by: Patrick Hodoul <patrick.hodoul@autodesk.com>

Co-authored-by: Rémi Achard <remiachard@gmail.com>
@remia remia deleted the fix/cmake-target-headers branch September 25, 2021 13:23
@zachlewis
Copy link
Collaborator

Since I'm not sure that I've been able to reproduce the undesired behavior on macos, I may not be the best person to review this PR, but I will say that OCIO builds and installs correctly with this PR.

This said, I feel like at some point recently -- possibly as a result of locally merging an earlier WIP of this PR -- I did have to slightly modify my build script to provide better hints to CMake for finding my preferred Python interpreter. I wish I had kept better notes, or had a better memory, or both. I could be getting confused with stuff I had to update in my OpenImageIO build script.

But to reiterate my earlier point, this builds and installs as it should, when using CMake best practices vis-a-vis python interpreter hints.

@remia
Copy link
Collaborator Author

remia commented Oct 18, 2021

@zachlewis I believe you meant to reply on #1510 ?

I described some ways to reproduce the issues here which is not a common situation I guess.

@zachlewis
Copy link
Collaborator

Oh whoops -- yep, that's the one I was going for.

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.

4 participants