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

[chronoengine] New port #29070

Merged
merged 2 commits into from
Feb 1, 2023
Merged

[chronoengine] New port #29070

merged 2 commits into from
Feb 1, 2023

Conversation

talregev
Copy link
Contributor

Describe the pull request
Add chrono port.

  • What does your PR fix?

    Fixes #...

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    <all / linux, windows, ...>, <Yes/No>

  • Does your PR follow the maintainer guide?

    Your answer

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@FrankXie05 FrankXie05 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jan 20, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 20, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 20, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 20, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 20, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 20, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 20, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 20, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 20, 2023
@talregev talregev marked this pull request as ready for review January 20, 2023 15:06
@talregev
Copy link
Contributor Author

talregev commented Jan 20, 2023

@FrankXie05
Please review.

vicroms
vicroms previously approved these changes Jan 23, 2023
@vicroms vicroms added the info:reviewed Pull Request changes follow basic guidelines label Jan 23, 2023
@talregev
Copy link
Contributor Author

There many modules that are optional. Some of them required matlab or cuda that I don't have to check.

OK then we should patch the build system or set CMAKE_DISABLE_FIND_PACKAGE_Xxx or similar to ensure those detections always say "false" so that people don't get a different thing installed if they happen to have matlab or cuda present on their system. We don't care that every optional knob is available when something is added, just that people don't randomly get extra knobs we haven't tested turned on just because they happened to build on a different machine.

@BillyONeal Chrono just release the 8.0.0 version. So I switch back to 8.

Excellent, thank you!

As far I understand, the optional detection is only activate if I enable the modules. All the module is off by default. You can take a look in their module documentations: https://api.projectchrono.org/development/tutorial_table_of_content_install.html

@BillyONeal Let me know if you agree with me.

@BillyONeal BillyONeal changed the title [chrono] New port [chronoengine] New port Jan 27, 2023
@BillyONeal
Copy link
Member

As far I understand, the optional detection is only activate if I enable the modules. All the module is off by default. You can take a look in their module documentations: https://api.projectchrono.org/development/tutorial_table_of_content_install.html

@BillyONeal Let me know if you agree with me.

There are many many unguarded find_package calls. Even just looking at 8.0.0-6dccb3d031.clean\src\CMakeLists.txt has:

find_package("TBB")
find_package(SIMD) # Guarded by an on by default option USE_SIMD, there is a port for this
find_package(Eigen3 3.3.0) # OK, an explicit dependency
find_package(MPI)
find_package(CUDA QUIET)
find_package(Thrust)
find_package(HDF5 COMPONENTS CXX) # OK, guarded by off-by-default option ENABLE_HDF5

This stuff needs to not build a subtly different package because someone happens to have some of these installed first.

@BillyONeal
Copy link
Member

(To clarify, you don't need to mess with every optional dependency that is indeed guarded by an off option. You do, however, need to demonstrate that those are so guarded. Patching more things to be REQUIRED is always going to be appreciated even if they happen to be not touched by that particular build, because it clearly demonstrates that something is, in fact, disabled)

@talregev
Copy link
Contributor Author

(To clarify, you don't need to mess with every optional dependency that is indeed guarded by an off option. You do, however, need to demonstrate that those are so guarded. Patching more things to be REQUIRED is always going to be appreciated even if they happen to be not touched by that particular build, because it clearly demonstrates that something is, in fact, disabled)

I will create a patch.

github-actions[bot]
github-actions bot previously approved these changes Jan 27, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 27, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 27, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 27, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 27, 2023
@talregev
Copy link
Contributor Author

@BillyONeal Please review.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

.\vcpkg.exe install chronoengine:x64-windows pass
.\vcpkg.exe install chronoengine[irrlicht]:x64-windows pass
.\vcpkg.exe install chronoengine[vehicle]:x64-windows pass
.\vcpkg.exe install chronoengine[irrlicht,vehicle]:x64-windows pass

Thanks for the new port!

@BillyONeal BillyONeal merged commit b63f61e into microsoft:master Feb 1, 2023
@talregev talregev deleted the TalR/chrono branch February 1, 2023 07:17
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants