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

Replace exec_program with execute_process #402

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Jan 18, 2024

🦟 Bug fix

Summary

In cmake 3.28, using exec_program causes a warning. The command has been deprecated since 3.0 and execute_process is the recommended replacement.

The changes were not trivial, so I thought we can merge in main, test it across all the libraries and then backport to gz-cmake3.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Jan 18, 2024
@mjcarroll
Copy link
Contributor

Out of curiosity, are we using the CheckSSE results anywhere? Seems like it may be unnecessary in modern versions of CMake unless we are doing something really wild and crazy with intrinsics somewhere.

@azeey
Copy link
Contributor Author

azeey commented Jan 18, 2024

Out of curiosity, are we using the CheckSSE results anywhere? Seems like it may be unnecessary in modern versions of CMake unless we are doing something really wild and crazy with intrinsics somewhere.

We are setting flags based on the results here:
https://github.com/azeey/gz-cmake/blob/ede16166069aed07ae51452b9395113499b59959/cmake/GzSetCompilerFlags.cmake#L244-L259

Are you saying those flags would be set automatically by modern versions of cmake?

@mjcarroll
Copy link
Contributor

mjcarroll commented Jan 18, 2024

Are you saying those flags would be set automatically by modern versions of cmake?

I thought that modern cmake versions actually included this, but it looks like the answer is even more complicated than that.

For source builds, it's probably fine what we are doing, but packaging is a different story.

It looks like each platform has a baseline specification, and it's up to us to handle any divergence from that baseline (for packaging). One of the recommended approaches is via: https://github.com/simd-everywhere/simde

Basically, there is a chance that we build with something like SSE4 turned on, but the deb won't work if the user doesn't have an SSE4 compatible processor. There is more information about what the baselines are here for Ubuntu: https://ubuntu.com/blog/optimising-ubuntu-performance-on-amd64-architecture
and Debian here: https://wiki.debian.org/ArchitectureSpecificsMemo#Architecture_baselines

@mjcarroll
Copy link
Contributor

All that to say, I don't think that has any impact on this PR... carry on.

@azeey azeey merged commit e3f3998 into gazebosim:main Jan 19, 2024
6 checks passed
j-rivero pushed a commit that referenced this pull request Feb 2, 2024
Signed-off-by: Addisu Z. Taddese <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
azeey added a commit that referenced this pull request Feb 2, 2024
Signed-off-by: Addisu Z. Taddese <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
azeey added a commit to azeey/gz-cmake that referenced this pull request May 3, 2024
Signed-off-by: Addisu Z. Taddese <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
azeey added a commit that referenced this pull request May 3, 2024
* Replace exec_program with execute_process (#402)
* Remove additional exec_program

Signed-off-by: Addisu Z. Taddese <[email protected]>

---------

Signed-off-by: Addisu Z. Taddese <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants