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

[gz-rendering7] new port #34618

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Oct 20, 2023

  • Changes comply with the maintainer guide
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@jimwang118 jimwang118 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Oct 20, 2023
@talregev talregev force-pushed the TalR/gz-rendering7 branch 4 times, most recently from c4c0233 to 07ff610 Compare October 23, 2023 15:46
@talregev talregev requested a review from dg0yt October 23, 2023 16:02
@talregev talregev force-pushed the TalR/gz-rendering7 branch 2 times, most recently from 0c44d41 to e48a83a Compare October 23, 2023 23:00
@jimwang118
Copy link
Contributor

Usage test pass with following triplets:

x86-windows
x64-windows
x64-windows-static

@talregev
Copy link
Contributor Author

it still works in progress

@talregev talregev marked this pull request as draft October 24, 2023 07:52
@dg0yt
Copy link
Contributor

dg0yt commented Oct 24, 2023

Do not merge.

@talregev
Copy link
Contributor Author

@dg0yt I got an error on linux on this port. I will report it here when I can.

@talregev
Copy link
Contributor Author

@dg0yt Can I ask your help?
When I compile this port for linux, when it find ogre2, it search also for terra:

CMake Error: install(EXPORT "gz-rendering7-ogre2" ...) includes target "gz-rendering7-ogre2" which requires target "terra" that is not in any export set.
CMake Error in ogre2/src/CMakeLists.txt:
  export called with target "gz-rendering7-ogre2" which requires target
  "terra" that is not in any export set.

I am not sure how to solve it. This error is not connected to your changes.
I am sharing here logs.
gz-rendering7.zip

@talregev
Copy link
Contributor Author

talregev commented Oct 25, 2023

@jimwang118 @dg0yt @MonicaLiu0311
The ci skip the gz-rendering7 port because it depend on ogre2 port that skip by request in the ci.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 25, 2023

you refer to ogre-next... yes, depending on it means having no CI. And not getting a new port.
I don't know the state of the ogre world. Maybe at least one triplet could be switched.

You need to export terra now.

I won't do anything before #34667 is merged. I look at one problem and find two more. This doesn't scale well...

@talregev
Copy link
Contributor Author

you refer to ogre-next... yes, depending on it means having no CI. And not getting a new port. I don't know the state of the ogre world. Maybe at least one triplet could be switched.

You need to export terra now.

I won't do anything before #34667 is merged. I look at one problem and find two more. This doesn't scale well...

I am waiting for your PR will merge.
meanwhile I got this problem and I check this before your fix and it also happen.
Also the terra target is created inside the gz-rendering7.
How I can export this target?
https://github.com/gazebosim/gz-rendering/blob/gz-rendering7_7.4.1/ogre2/src/terrain/Terra/CMakeLists.txt#L9

@jimwang118
Copy link
Contributor

you refer to ogre-next... yes, depending on it means having no CI. And not getting a new port. I don't know the state of the ogre world. Maybe at least one triplet could be switched.
You need to export terra now.
I won't do anything before #34667 is merged. I look at one problem and find two more. This doesn't scale well...

I am waiting for your PR will merge. meanwhile I got this problem and I check this before your fix and it also happen. Also the terra target is created inside the gz-rendering7. How I can export this target? https://github.com/gazebosim/gz-rendering/blob/gz-rendering7_7.4.1/ogre2/src/terrain/Terra/CMakeLists.txt#L9

You should use the install command to export terra.

@talregev talregev force-pushed the TalR/gz-rendering7 branch 3 times, most recently from 28ccacc to 6fec729 Compare October 28, 2023 06:32
@talregev
Copy link
Contributor Author

you refer to ogre-next... yes, depending on it means having no CI. And not getting a new port. I don't know the state of the ogre world. Maybe at least one triplet could be switched.
You need to export terra now.
I won't do anything before #34667 is merged. I look at one problem and find two more. This doesn't scale well...

I am waiting for your PR will merge. meanwhile I got this problem and I check this before your fix and it also happen. Also the terra target is created inside the gz-rendering7. How I can export this target? https://github.com/gazebosim/gz-rendering/blob/gz-rendering7_7.4.1/ogre2/src/terrain/Terra/CMakeLists.txt#L9

You should use the install command to export terra.

@jimwang118 @dg0yt

I added similar line as here:
https://github.com/gazebosim/gz-rendering/blob/gz-rendering7_7.4.1/ogre2/src/CMakeLists.txt#L63
install(TARGETS "terra" DESTINATION ${GZ_RENDERING_ENGINE_INSTALL_DIR})

And I still get the same error. What should I do?

@dg0yt
Copy link
Contributor

dg0yt commented Oct 29, 2023

CMake Error in ogre2/src/CMakeLists.txt:
 export called with target "gz-rendering7-ogre2" which requires target
 "terra" that is not in any export set.

You need install(TARGETS terrra EXPORT <export_set> ...).
Then install(EXPORT <export_set> ...) will be happy.

@talregev
Copy link
Contributor Author

@dg0yt Thank you for your help.
I try:

install(TARGETS "terra" EXPORT terra)
install(EXPORT terra DESTINATION ${GZ_RENDERING_ENGINE_INSTALL_DIR})

And I got the same error.
I also try with different names for export_set.

@talregev talregev marked this pull request as ready for review October 30, 2023 04:00
@talregev
Copy link
Contributor Author

@dg0yt I got it. Thank you for your help.
@jimwang118 ready for review.

@talregev talregev marked this pull request as draft October 30, 2023 20:23
@talregev talregev marked this pull request as ready for review October 30, 2023 20:43
@talregev
Copy link
Contributor Author

@jimwang118 It ready for review.

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Oct 31, 2023
@JavierMatosD JavierMatosD merged commit 0e2a0e0 into microsoft:master Oct 31, 2023
15 checks passed
@talregev talregev deleted the TalR/gz-rendering7 branch October 31, 2023 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants