-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[osg] Fix plugin installation and target locations #26214
Conversation
This change removes the stage in portfile.cmake which relocates the osg plugins from bin/ to plugins/. Instead, the OsgMacroUtils.cmake file is patched to install directly to the plugins folder. This has two primary effects. The first is correcting the exported target locations, as they still pointed to bin/ after moving the plugins. The second is fixing the process on non-Windows platforms, as the plugins were not being moved in those cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have modified or added at least one vcpkg.json where you should check the license
field.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/osg/vcpkg.json
Valid values for the license field can be found in the documentation
Pinging @cbrl for response. Is work still being done for this PR? |
Sorry for the delay. I don't believe any additional work is required, and I have successfully utilized the port on Windows and Linux with these modifications. I initially opened this as a draft because I was unsure as to how the plugins should be handled across different platforms and library linkage, but the discussion with @dg0yt in the issue linked above suggests that this is the correct path. Absent any recommended changes to this solution, I am ready to mark it for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have modified or added at least one vcpkg.json where you should check the license
field.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/osg/vcpkg.json
Valid values for the license field can be found in the documentation
Depend on #26698
|
I think this fix makes sense. Have we tested if the |
@dan-shaw AFAIK there is no generic mechanism for deploying plugins, only specific hooks for some packages (Qt, OpenNI2, Magnum, AzureKinectSensorSDK). There are many more ports with plugins (osg, pdal, ogre, ...). |
7bd9f43
I haven't explicitly tested the Visual Studio integration, but from memory it does not have any knowledge of the plugins folder. However, as @dg0yt mentioned, there doesn't seem to be a well-defined mechanism for plugin deployment that it would integrate with. As-is, this PR doesn't change that behavior. I think that would be outside the scope, and would require changes to multiple ports for consistency. |
Describe the pull request
This change removes the stage in portfile.cmake which relocates the osg plugins from bin/ to plugins/. Instead, the OsgMacroUtils.cmake file is patched to install directly to the plugins folder.
#25919 contains some discussion on the issue that this changes addresses. This is opened as a draft since I was not entirely clear on the most desirable outcome. I believe this is the simplest method that achieves the results, but there may be a more preferred method or alternative location for the plugins.
What does your PR fix?
This has two primary effects. The first is correcting the exported target locations in unofficial-osg, as they still pointed to bin/ after moving the plugins. The second is fixing the process on non-Windows triplets using dynamic linkage, as the plugins were not being moved in those cases.
Which triplets are supported/not supported? Have you updated the CI baseline?
All, No
Does your PR follow the maintainer guide?
Yes
If you have added/updated a port: Have you run
./vcpkg x-add-version --all
and committed the result?Yes