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

Fixed macos symbol in common::profiler #262

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Oct 21, 2021

Signed-off-by: Alejandro Hernández [email protected]

🦟 Bug fix

Summary

While I was testing the ign-gazebo python interface on macos I noticed that some symbols are missing. This should fix it.

There are some other which are not included, for example: RMT_USE_D3D11 or RMT_USE_OPENGL. Should we need to remove these ones or add the target_compile_definitions?

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

Signed-off-by: Alejandro Hernández <[email protected]>
@ahcorde ahcorde requested a review from mjcarroll October 21, 2021 07:45
@ahcorde ahcorde self-assigned this Oct 21, 2021
@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Oct 21, 2021
@codecov
Copy link

codecov bot commented Oct 21, 2021

Codecov Report

Merging #262 (b81e1f6) into ign-common3 (df11d6e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-common3     #262   +/-   ##
============================================
  Coverage        76.55%   76.55%           
============================================
  Files               73       73           
  Lines            10386    10386           
============================================
  Hits              7951     7951           
  Misses            2435     2435           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df11d6e...b81e1f6. Read the comment docs.

@mjcarroll
Copy link
Contributor

There are some other which are not included, for example: RMT_USE_D3D11 or RMT_USE_OPENGL. Should we need to remove these ones or add the target_compile_definitions?

I didn't expose these because I didn't want to give the impression that they worked or were supported. We only needed CPU tracing as far as I remember when we first added the profiler.

@ahcorde
Copy link
Contributor Author

ahcorde commented Oct 21, 2021

There are some other which are not included, for example: RMT_USE_D3D11 or RMT_USE_OPENGL. Should we need to remove these ones or add the target_compile_definitions?

I didn't expose these because I didn't want to give the impression that they worked or were supported. We only needed CPU tracing as far as I remember when we first added the profiler.

maybe it's a good idea to remove them ? I can create another PR for that.

This one is required by macos

@chapulina chapulina added the macOS macOS support label Oct 25, 2021
@chapulina chapulina merged commit c649e64 into ign-common3 Nov 9, 2021
@chapulina chapulina deleted the ahcorde/fix/macos_symbol branch November 9, 2021 02:04
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-24-citadel-edifice-fortress/1241/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🔮 dome Ignition Dome Gazebo 1️1️ Dependency of Gazebo classic version 11 macOS macOS support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants