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

Use CMake visibility flags #1411

Merged

Conversation

remia
Copy link
Collaborator

@remia remia commented Jun 13, 2021

Following recent discussions around symbol visibility, we noticed that OpenColorIO only seem to use -fvisibility-inlines-hidden but not -fvisibility=hidden on the core library. This PR add it and replace the hard coded compiler flags with CMake utility variables to enable that behaviour by default overall.

Here is some tests I did on macOS to illustrate the impact:

cmake -DCMAKE_INSTALL_PREFIX=myinstall -DOCIO_BUILD_APPS=ON -DOCIO_INSTALL_EXT_PACKAGES=ALL .. && cmake --build . -j 12 && make install

// A few moments later..
nm myinstall/lib/libOpenColorIO.2.1.0.dylib | grep " T " | grep -i _XML_ | wc -l
nm myinstall/lib/libOpenColorIO.2.1.0.dylib | grep " T " | wc -l
ls -lh myinstall/lib/libOpenColorIO.2.1.0.dylib | awk '{print $5}'

MASTER BRANCH
66
3318
6.6M

THIS PR
66
907
6.3M

Notice that Expat symbols are still leaked, I don't know yet how to fix that on macOS.

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

@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.

This is a good improvement that (along with the related PR I made) aligns OCIO with best practices for symbol hiding. Please do also backport this to RB-2.0! Many of us would be relieved to get a 2.0.2 that does all the symbol hiding properly.

@lgritz
Copy link
Collaborator

lgritz commented Jun 13, 2021

I don't know the right solution for macos, but I would certainly like to find out. What I tried in the other PR works for Linux, but that linker option just didn't exist on Mac.

Copy link
Member

@hodoulp hodoulp left a comment

Choose a reason for hiding this comment

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

@remia I'm waiting last changes about visibility default values (if that's a valid comment / approach) before approving the pull request. Let me know.

remia and others added 3 commits June 14, 2021 21:24
Signed-off-by: Rémi Achard <remiachard@gmail.com>
Signed-off-by: Rémi Achard <remiachard@gmail.com>
@hodoulp hodoulp merged commit 85a22e2 into AcademySoftwareFoundation:master Jun 15, 2021
hodoulp added a commit to autodesk-forks/OpenColorIO that referenced this pull request Jun 28, 2021
* Update install doc minimal version numbers

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

* Use CMake visibility presets variables

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

* Fix python package case

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

* Allow advanced user to override symbol visibility

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

Co-authored-by: Patrick Hodoul <patrick.hodoul@autodesk.com>
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>
hodoulp added a commit that referenced this pull request Jun 28, 2021
* Update install doc minimal version numbers

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

* Use CMake visibility presets variables

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

* Fix python package case

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

* Allow advanced user to override symbol visibility

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

Co-authored-by: Patrick Hodoul <patrick.hodoul@autodesk.com>
doug-walker added a commit that referenced this pull request Jul 6, 2021
* Adsk Contrib - Improve file rules for v1 configs

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Adsk Contrib - Emergency GPU build fix (#1391)

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Adsk Contrib - Fix some bugs found by Maya and SonarCloud (#1403)

* Adsk Contrib - Fix some bugs found by Maya and SonarCloud

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Fix a Windows warning

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* After Effects and Photoshop plug-in updates (#1373)

* Set Mac OS deployment target to 10.10

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* More 10.10 targets

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Ditch ADD_EXTRA_BUILTINS

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Getting strange callback when AE effect is pasted

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Use NDEBUG in AE project

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Disable radio buttons with no config

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Update version

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Move to AE 2021 SDK, set PF_OutFlag2_MUTABLE_RENDER_SEQUENCE_DATA_SLOWER

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Define OCIO_DEPRECATED in After Effects builds

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Verify change from color space menu

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Handle situation where color space has a '/' in it

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Windows family separator fix

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Invert everything

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Invert everything Windows

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Disable more controls

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Incorporate invert into LUT export

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Get rid of fullPaths stuff

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Fix changed config settings retention

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

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

* Add OpenColorIOConfig.cmake generation (#1397)

* Initial OpenColorIO Config CMake implementation

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

* Remove macro, improve documentation

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

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

* Use CMake visibility flags (#1411)

* Update install doc minimal version numbers

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

* Use CMake visibility presets variables

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

* Fix python package case

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

* Allow advanced user to override symbol visibility

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

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

* Fix Windows build

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Update include/OpenColorIO/OpenColorIO.h

Co-authored-by: Michael Dolan <michdolan@gmail.com>
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Update src/OpenColorIO/FileRules.cpp

Co-authored-by: Michael Dolan <michdolan@gmail.com>
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Hide dependency symbol visibility (#1409) (#1416)

When creating libOpenColorIO.so, we lacked the linker commands that
hide symbol visibility from the dependent libraries, which is
necessary to prevent OCIO from exporting the symbols from Expat and
the other dependencies that OCIO needs to use internally but is not
trying intentionally to expose via its API.

Failing to do this can result in symbol clashes and all sorts of
subtle errors if OCIO is used in an app that also uses and is linked
against a potentially different version of Expat (or any of the other
deps).

Signed-off-by: Larry Gritz <lg@larrygritz.com>

Co-authored-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Adsk Contrib - Improve DX11 & Cg fragment shader language support (#1406)

* Adsk Contrib - Improve DX11 support

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Improve Cg support

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Improve GPU comments

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Fix unit test failures

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

Co-authored-by: Brendan Bolles <brendan@fnordware.com>
Co-authored-by: Rémi Achard <remiachard@gmail.com>
Co-authored-by: Michael Dolan <michdolan@gmail.com>
Co-authored-by: Larry Gritz <lg@larrygritz.com>
Co-authored-by: doug-walker <43830961+doug-walker@users.noreply.github.com>
doug-walker added a commit that referenced this pull request Jul 13, 2021
* Update install doc minimal version numbers

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

* Use CMake visibility presets variables

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

* Fix python package case

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

* Allow advanced user to override symbol visibility

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>
Co-authored-by: doug-walker <43830961+doug-walker@users.noreply.github.com>
hodoulp added a commit that referenced this pull request Aug 2, 2021
* Adsk Contrib - Improve file rules for v1 configs

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Adsk Contrib - Emergency GPU build fix (#1391)

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Adsk Contrib - Fix some bugs found by Maya and SonarCloud (#1403)

* Adsk Contrib - Fix some bugs found by Maya and SonarCloud

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Fix a Windows warning

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* After Effects and Photoshop plug-in updates (#1373)

* Set Mac OS deployment target to 10.10

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* More 10.10 targets

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Ditch ADD_EXTRA_BUILTINS

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Getting strange callback when AE effect is pasted

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Use NDEBUG in AE project

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Disable radio buttons with no config

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Update version

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Move to AE 2021 SDK, set PF_OutFlag2_MUTABLE_RENDER_SEQUENCE_DATA_SLOWER

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Define OCIO_DEPRECATED in After Effects builds

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Verify change from color space menu

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Handle situation where color space has a '/' in it

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Windows family separator fix

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Invert everything

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Invert everything Windows

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Disable more controls

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Incorporate invert into LUT export

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Get rid of fullPaths stuff

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Fix changed config settings retention

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

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

* Add OpenColorIOConfig.cmake generation (#1397)

* Initial OpenColorIO Config CMake implementation

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

* Remove macro, improve documentation

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

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

* Use CMake visibility flags (#1411)

* Update install doc minimal version numbers

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

* Use CMake visibility presets variables

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

* Fix python package case

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

* Allow advanced user to override symbol visibility

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

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

* Fix Windows build

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Update include/OpenColorIO/OpenColorIO.h

Co-authored-by: Michael Dolan <michdolan@gmail.com>
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Update src/OpenColorIO/FileRules.cpp

Co-authored-by: Michael Dolan <michdolan@gmail.com>
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Hide dependency symbol visibility (#1409) (#1416)

When creating libOpenColorIO.so, we lacked the linker commands that
hide symbol visibility from the dependent libraries, which is
necessary to prevent OCIO from exporting the symbols from Expat and
the other dependencies that OCIO needs to use internally but is not
trying intentionally to expose via its API.

Failing to do this can result in symbol clashes and all sorts of
subtle errors if OCIO is used in an app that also uses and is linked
against a potentially different version of Expat (or any of the other
deps).

Signed-off-by: Larry Gritz <lg@larrygritz.com>

Co-authored-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Adsk Contrib - Improve DX11 & Cg fragment shader language support (#1406)

* Adsk Contrib - Improve DX11 support

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Improve Cg support

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Improve GPU comments

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Fix unit test failures

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

Co-authored-by: Brendan Bolles <brendan@fnordware.com>
Co-authored-by: Rémi Achard <remiachard@gmail.com>
Co-authored-by: Michael Dolan <michdolan@gmail.com>
Co-authored-by: Larry Gritz <lg@larrygritz.com>
Co-authored-by: doug-walker <43830961+doug-walker@users.noreply.github.com>
hodoulp added a commit that referenced this pull request Aug 12, 2021
…t for v1 (#1445)

* Adsk Contrib - Improve File Rules support for v1 configs (#1417)

* Adsk Contrib - Improve file rules for v1 configs

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Adsk Contrib - Emergency GPU build fix (#1391)

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Adsk Contrib - Fix some bugs found by Maya and SonarCloud (#1403)

* Adsk Contrib - Fix some bugs found by Maya and SonarCloud

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Fix a Windows warning

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* After Effects and Photoshop plug-in updates (#1373)

* Set Mac OS deployment target to 10.10

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* More 10.10 targets

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Ditch ADD_EXTRA_BUILTINS

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Getting strange callback when AE effect is pasted

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Use NDEBUG in AE project

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Disable radio buttons with no config

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Update version

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Move to AE 2021 SDK, set PF_OutFlag2_MUTABLE_RENDER_SEQUENCE_DATA_SLOWER

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Define OCIO_DEPRECATED in After Effects builds

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Verify change from color space menu

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Handle situation where color space has a '/' in it

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Windows family separator fix

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Invert everything

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Invert everything Windows

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Disable more controls

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Incorporate invert into LUT export

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Get rid of fullPaths stuff

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

* Fix changed config settings retention

Signed-off-by: Brendan Bolles <brendan@fnordware.com>

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

* Add OpenColorIOConfig.cmake generation (#1397)

* Initial OpenColorIO Config CMake implementation

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

* Remove macro, improve documentation

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

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

* Use CMake visibility flags (#1411)

* Update install doc minimal version numbers

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

* Use CMake visibility presets variables

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

* Fix python package case

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

* Allow advanced user to override symbol visibility

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

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

* Fix Windows build

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Update include/OpenColorIO/OpenColorIO.h

Co-authored-by: Michael Dolan <michdolan@gmail.com>
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Update src/OpenColorIO/FileRules.cpp

Co-authored-by: Michael Dolan <michdolan@gmail.com>
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Hide dependency symbol visibility (#1409) (#1416)

When creating libOpenColorIO.so, we lacked the linker commands that
hide symbol visibility from the dependent libraries, which is
necessary to prevent OCIO from exporting the symbols from Expat and
the other dependencies that OCIO needs to use internally but is not
trying intentionally to expose via its API.

Failing to do this can result in symbol clashes and all sorts of
subtle errors if OCIO is used in an app that also uses and is linked
against a potentially different version of Expat (or any of the other
deps).

Signed-off-by: Larry Gritz <lg@larrygritz.com>

Co-authored-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Adsk Contrib - Improve DX11 & Cg fragment shader language support (#1406)

* Adsk Contrib - Improve DX11 support

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Improve Cg support

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Improve GPU comments

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

* Fix unit test failures

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

Co-authored-by: Brendan Bolles <brendan@fnordware.com>
Co-authored-by: Rémi Achard <remiachard@gmail.com>
Co-authored-by: Michael Dolan <michdolan@gmail.com>
Co-authored-by: Larry Gritz <lg@larrygritz.com>
Co-authored-by: doug-walker <43830961+doug-walker@users.noreply.github.com>

* Add missing file change

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

Co-authored-by: Brendan Bolles <brendan@fnordware.com>
Co-authored-by: Rémi Achard <remiachard@gmail.com>
Co-authored-by: Michael Dolan <michdolan@gmail.com>
Co-authored-by: Larry Gritz <lg@larrygritz.com>
Co-authored-by: doug-walker <43830961+doug-walker@users.noreply.github.com>
@remia remia deleted the feature/cmake_improvements branch August 24, 2021 11:08
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