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

[osg] Fix plugin installation and target locations #26214

Merged
merged 10 commits into from
Oct 9, 2022
Merged

[osg] Fix plugin installation and target locations #26214

merged 10 commits into from
Oct 9, 2022

Conversation

cbrl
Copy link
Contributor

@cbrl cbrl commented Aug 7, 2022

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

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.
Copy link

@github-actions github-actions bot left a 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

@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label Aug 8, 2022
@JonLiu1993
Copy link
Member

Pinging @cbrl for response. Is work still being done for this PR?

@cbrl
Copy link
Contributor Author

cbrl commented Sep 9, 2022

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.

@cbrl cbrl marked this pull request as ready for review September 12, 2022 16:03
github-actions[bot]
github-actions bot previously approved these changes Sep 13, 2022
Copy link

@github-actions github-actions bot left a 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

@JonLiu1993 JonLiu1993 added the depends:different-pr This PR or Issue depends on a PR which has been filed label Sep 13, 2022
@JonLiu1993
Copy link
Member

Depend on #26698

CMake Error: try_run() invoked in cross-compiling mode, please set the following cache variables appropriately:
   _OPENTHREADS_ATOMIC_USE_WIN32_INTERLOCKED_EXITCODE (advanced)
   _OPENTHREADS_ATOMIC_USE_WIN32_INTERLOCKED_EXITCODE__TRYRUN_OUTPUT (advanced)

@JackBoosY JackBoosY removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Sep 15, 2022
github-actions[bot]
github-actions bot previously approved these changes Sep 15, 2022
github-actions[bot]
github-actions bot previously approved these changes Sep 15, 2022
JonLiu1993
JonLiu1993 previously approved these changes Sep 16, 2022
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Sep 16, 2022
@dan-shaw dan-shaw added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 19, 2022
@dan-shaw
Copy link
Contributor

I think this fix makes sense. Have we tested if the applocal.ps1 (for Visual Studio integration) script has the correct behavior for the plugins folder?

@dan-shaw dan-shaw added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Sep 26, 2022
@dg0yt
Copy link
Contributor

dg0yt commented Sep 26, 2022

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

@cbrl cbrl dismissed stale reviews from JonLiu1993 and github-actions[bot] via 7bd9f43 October 4, 2022 15:21
@cbrl
Copy link
Contributor Author

cbrl commented Oct 4, 2022

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.

@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Oct 8, 2022
@vicroms vicroms merged commit 472e9f1 into microsoft:master Oct 9, 2022
@cbrl cbrl deleted the osg branch November 6, 2022 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants