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

Topic/move headers #2070

Merged
merged 13 commits into from Oct 25, 2023
Merged

Topic/move headers #2070

merged 13 commits into from Oct 25, 2023

Conversation

jorisv
Copy link
Contributor

@jorisv jorisv commented Oct 19, 2023

Pinocchio headers in repository are not organized as installed ones.

In this merge request, includes and sources are split to have a standard CMake/C++ project layout.
Headers in the repository and installed ones have the same organization.

This will avoid to have to create symlinks and will solve #2069

@jorisv jorisv added the bug label Oct 19, 2023
@jorisv jorisv requested a review from nim65s October 19, 2023 13:12
@jorisv jorisv self-assigned this Oct 19, 2023
bindings/python/CMakeLists.txt Show resolved Hide resolved
bindings/python/CMakeLists.txt Show resolved Hide resolved
bindings/python/parsers/python/model.cpp Show resolved Hide resolved
unittest/python_parser.cpp Show resolved Hide resolved
Copy link
Contributor

@nim65s nim65s left a comment

Choose a reason for hiding this comment

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

Using GLOB / GLOB_RECURSE to find source files is a wrong CMake practice, and I think we should use this PR to fix this.

  1. This glob will be expanded only on the first configure call. Therefore, when someone with an existing successful build dir run git pull and get new source files, those new sources will be ignored by CMake, leading to hard to understand bugs
  2. This glob would also include unwanted temporary files or archive artifacts

(eg., on NetBSD, with a .tar.gz created on mac osx:

$ wget https://github.com/stack-of-tasks/pinocchio/releases/download/v2.6.20/pinocchio-2.6.20.tar.gz
$ gzip -d pinocchio-2.6.20.tar.gz
$ pax -r < pinocchio-2.6.20.tar
$ find pinocchio-2.6.20/src/core/ -name \*.hpp                                                                               
pinocchio-2.6.20/src/core/binary-op.hpp
pinocchio-2.6.20/src/core/PaxHeader/binary-op.hpp
pinocchio-2.6.20/src/core/PaxHeader/unary-op.hpp
pinocchio-2.6.20/src/core/unary-op.hpp

)
3. jrl-cmakemodule install public headers automatically when they are defined in a standard way

I understand this is boring to write the list of all the files we want, but this would help in:

  • not having to write regex to conditionally exclude some files
  • not having to create directories / symlink files around
  • not having to write a custom function to install those

refs.:
https://github.com/jrl-umi3218/jrl-cmakemodules/blob/master/header.cmake
https://cmake.org/cmake/help/latest/command/file.html#filesystem

Note: We do not recommend using GLOB to collect a list of source files from your source tree

@jorisv jorisv closed this Oct 20, 2023
@jorisv jorisv reopened this Oct 20, 2023
@jorisv
Copy link
Contributor Author

jorisv commented Oct 20, 2023

Using GLOB / GLOB_RECURSE to find source files is a wrong CMake practice, and I think we should use this PR to fix this.

1. This glob will be expanded only on the first configure call. Therefore, when someone with an existing successful `build` dir run `git pull` and get new source files, those new sources will be ignored by CMake, leading to hard to understand bugs

2. This glob would also include unwanted temporary files or archive artifacts

(eg., on NetBSD, with a .tar.gz created on mac osx:

$ wget https://github.com/stack-of-tasks/pinocchio/releases/download/v2.6.20/pinocchio-2.6.20.tar.gz
$ gzip -d pinocchio-2.6.20.tar.gz
$ pax -r < pinocchio-2.6.20.tar
$ find pinocchio-2.6.20/src/core/ -name \*.hpp                                                                               
pinocchio-2.6.20/src/core/binary-op.hpp
pinocchio-2.6.20/src/core/PaxHeader/binary-op.hpp
pinocchio-2.6.20/src/core/PaxHeader/unary-op.hpp
pinocchio-2.6.20/src/core/unary-op.hpp

) 3. jrl-cmakemodule install public headers automatically when they are defined in a standard way

I understand this is boring to write the list of all the files we want, but this would help in:

* not having to write regex to conditionally exclude some files

* not having to create directories / symlink files around

* not having to write a custom function to install those

refs.: https://github.com/jrl-umi3218/jrl-cmakemodules/blob/master/header.cmake https://cmake.org/cmake/help/latest/command/file.html#filesystem

Note: We do not recommend using GLOB to collect a list of source files from your source tree

I Agree on GLOB/GLOB_RECURSE pitfall.
Also the way we deleting files when options are not enabled is not pleasant and safe (it's easy to forgot).

I will have a look on jrl-cmakemodule header management.

@jorisv
Copy link
Contributor Author

jorisv commented Oct 20, 2023

@nim65s It's good, last commit remove GLOB/GLOB_RECURSE for sources. And it use ${PROJECT_NAME}_HEADERS to install the headers files.

Copy link
Contributor

@nim65s nim65s left a comment

Choose a reason for hiding this comment

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

This is really nice, thanks !
I still have a couple comments

sources.cmake Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
include/pinocchio/parsers/python.hpp Outdated Show resolved Hide resolved
Joris Vaillant and others added 3 commits October 23, 2023 13:35
@jorisv jorisv marked this pull request as ready for review October 24, 2023 12:16
@jorisv jorisv merged commit 5e0ed48 into stack-of-tasks:devel Oct 25, 2023
14 checks passed
nim65s added a commit to nim65s/robotpkg that referenced this pull request Dec 1, 2023
    ## [2.6.21] - 2023-11-27

    ### Added

    - Add inverse dynamics (`rnea`) Python and C++ example ([2083](stack-of-tasks/pinocchio#2083))
    - Add visualization of Frames in MeshCat viewer ([2098](stack-of-tasks/pinocchio#2098))

    ### Fixed

    - Re-initialize `Ycrb[0]` in `crbaMinimal` ([2040](stack-of-tasks/pinocchio#2040))
    - Fix custom scalar use in `log` function ([2047](stack-of-tasks/pinocchio#2047))
    - Raise exception on wrong input size in `XYZQUATToSE3` Python binding function ([2073](stack-of-tasks/pinocchio#2073))
    - Remove memory leak in `buildGeomFromUrdf` and `buildGeomFromUrdfString` Python binding functions ([2082]()stack-of-tasks/pinocchio#2082)
    - Fix Panda3D viewer examples ([2087](stack-of-tasks/pinocchio#2087))
    - Fix centroidal dynamics derivatives with respect to time ([2094](stack-of-tasks/pinocchio#2094)))

    ### Changed

    - Rename freeflyer_joint to root_joint in `humanoid` sample model ([2043](stack-of-tasks/pinocchio#2043))
    - CMake minimal version is now 3.10 ([2055](stack-of-tasks/pinocchio#2055))
    - Split headers and sources in different directories to have a more standard C++ project ([2070](stack-of-tasks/pinocchio#2070))

    ### Removed

    - Remove support to `hpp-fcl` < v2.0.0 ([2086](stack-of-tasks/pinocchio#2086))

    ## Packaging changes

    - removed patches ad, ae: fixed upstream 🎉
    - updated patches af, ah, an
    - added patch ao
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants