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

[joltphysics] Add new port #27715

Merged
merged 11 commits into from
Nov 15, 2022
Merged

[joltphysics] Add new port #27715

merged 11 commits into from
Nov 15, 2022

Conversation

RT2Code
Copy link
Contributor

@RT2Code RT2Code commented Nov 8, 2022

I know this is not what's advocated for vcpkg, but for this port, I'm using the latest commit instead of the latest release version. The reason for that is that since the previous release, the CMakelist.txt file have been improved a lot, supports many more platforms and it was far easier to make a working and clean port from it.

  • What does your PR fix?

Add new port joltphysics : https://github.com/jrouwe/JoltPhysics

  • 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

github-actions[bot]
github-actions bot previously approved these changes Nov 8, 2022
github-actions[bot]
github-actions bot previously approved these changes Nov 8, 2022
github-actions[bot]
github-actions bot previously approved these changes Nov 8, 2022
github-actions[bot]
github-actions bot previously approved these changes Nov 8, 2022
@RT2Code RT2Code marked this pull request as ready for review November 9, 2022 00:45
@JonLiu1993 JonLiu1993 changed the title [joltphysics] New port [joltphysics] Add new port Nov 9, 2022
@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Nov 9, 2022
versions/j-/joltphysics.json Outdated Show resolved Hide resolved
scripts/ci.baseline.txt Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Nov 9, 2022
JonLiu1993
JonLiu1993 previously approved these changes Nov 10, 2022
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Nov 10, 2022
@JavierMatosD
Copy link
Contributor

Hi @RT222, thank you for the PR!

You mentioned using the latest upstream commit instead of their latest release. However, we can't do this without upstreams approval. I went ahead and reached out to see if they approved or if they were willing to make an early release.

Additionally, I noticed that this port doesn't provide any usage instructions. Under some conditions,
vcpkg will generate the usage instructions, but this port does not. Would you mind adding them?

To do so,

  1. Create a file named usage in the port directory with the content you want to display.
    Here is an example using zlib:
The package zlib is compatible with built-in CMake targets:

    find_package(ZLIB REQUIRED)
    target_link_libraries(main PRIVATE ZLIB::ZLIB)
  1. To the portfile.cmake, add
file(INSTALL "${CMAKE_CURRENT_LIST_DIR}/usage" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}")
  1. Rerun vcpkg x-add-version

I will keep the info:reviewed label while I wait for upstream's response to make sure we don't drop this.

"description": "A multi core friendly rigid body physics and collision detection library suitable for games and VR applications",
"homepage": "https://github.com/jrouwe/JoltPhysics",
"license": "MIT",
"supports": "!(arm & uwp)",
Copy link

Choose a reason for hiding this comment

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

The library should support ARM and UWP now.

ports/joltphysics/portfile.cmake Show resolved Hide resolved
@jrouwe
Copy link

jrouwe commented Nov 11, 2022

I've created a new release of the library: v2.0.0.

@JavierMatosD
Copy link
Contributor

JavierMatosD commented Nov 11, 2022

@RT222 It looks like @jrouwe was kind enough to do a release for us, so please update the port and version to the latest release. Additionally, the library now supports arm and uwp. We can delete them from the support clause. Finally, the usage text file is not technically required, so that wouldn't block this PR. That said, it would be very helpful. :)

@JavierMatosD JavierMatosD added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Nov 11, 2022
@RT2Code
Copy link
Contributor Author

RT2Code commented Nov 12, 2022

Thanks @JavierMatosD for the assistance, and thanks @jrouwe for your reactivity and for sharing with us this awesome library.

There's still some issues to tackle before the port is ready :

  • Thank you for adding support for ARM and UWP. Unfortunately, I still can't build your library on this platform with vcpkg, it fails with the following compilation error :
    Jolt\Core\Core.h(109): fatal error C1189: #error: Unsupported CPU architecture
  • By default, vcpkg use the Debug and Release build type modes. In the JoltPhysics documentation, it is mentioned that the Release mode include support for profiling and debug rendering. Do you think it would be more appropriate to use the Distribution instead of the Release build type mode?

@jrouwe
Copy link

jrouwe commented Nov 12, 2022

Jolt\Core\Core.h(109): fatal error C1189: #error: Unsupported CPU architecture

By default it detects the ARM platform by: #if defined(__aarch64__) || defined(_M_ARM64). This works on my machine, but maybe _M_ARM64 is not set when building as a package for some reason (maybe it's compiling in emulation mode and also _M_ARM64EC needs to be checked)? What do I need to do to repro this?

Do you think it would be more appropriate to use the Distribution instead of the Release build type mode?

Yes, Distribution is better if there are only 2 choices to make.

@jrouwe
Copy link

jrouwe commented Nov 12, 2022

Ok, I know what is going on. It's trying to compile the 32-bit ARM version instead of the 64-bit version.

vcpkg install joltphysics:arm64-uwp

works,

vcpkg install joltphysics:arm-uwp

doesn't.

Let me see if a 32-bit version can work.

@jrouwe
Copy link

jrouwe commented Nov 12, 2022

The latest version on 'master' should work now. Please let me know if it works for you too and then I'll create a release with it.

@RT2Code
Copy link
Contributor Author

RT2Code commented Nov 12, 2022

I can confirm that it's working fine now, well done. :)

@jrouwe
Copy link

jrouwe commented Nov 12, 2022

Ok, I created release 2.0.1

@RT2Code
Copy link
Contributor Author

RT2Code commented Nov 12, 2022

I think it's ready to go.

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Nov 14, 2022
@BillyONeal
Copy link
Member

@RT222 Thanks again for the new port!
@jrouwe Thanks for being a great upstream maintainer and tagging meaningful releases!
@JavierMatosD Thanks for bringing these folks together!
@JonLiu1993 Thanks for the initial review!

@BillyONeal BillyONeal merged commit 1ecdbe8 into microsoft:master Nov 15, 2022
@RT2Code RT2Code deleted the joltphysics branch November 15, 2022 08:18
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.

5 participants