Don't use ignition/msgs.hh

[mjcarroll]

Using narrowly scoped <gz/msgs/foo> headers cuts compilation time on my computer:

Before:

**** Time summary:
Compilation (181 times):
  Parsing (frontend):          379.8 s
  Codegen & opts (backend):    164.9 s

After:

**** Time summary:
Compilation (180 times):
  Parsing (frontend):          110.9 s
  Codegen & opts (backend):     87.7 s

[codecov]

Codecov Report

Merging #315 (33eae91) into gz-transport12 (1956422) will not change coverage.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           gz-transport12     #315   +/-   ##
===============================================
  Coverage           88.11%   88.11%           
===============================================
  Files                  52       52           
  Lines                4848     4848           
===============================================
  Hits                 4272     4272           
  Misses                576      576           

Impacted Files	Coverage Δ
include/gz/transport/detail/Node.hh	92.42% <ø> (ø)
src/Clock.cc	100.00% <ø> (ø)
src/NodeShared.cc	77.76% <ø> (ø)
src/cmd/gz.cc	95.31% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[chapulina]

Yes! Thanks! We should almost never use the library_name.hh headers.

I believe we need to wrap those includes in warning suppressions though, for Windows.

[caguero]

I think there are extra warnings on Windows.

[mjcarroll]

I believe we need to wrap those includes in warning suppressions though, for Windows.

I have a fix coming for ign-msgs that should fix all 🤞

[mjcarroll]

Quick note, this will have downstream impacts. Node.hh was previously including <ignition/msgs/msgs.hh>, so anyone who was depending on message definitions being transitively included will be broken.

My preference would be to at least fix it in all of our libraries before merging this. I don't know if this could be considered an API/ABI break as it is depending on bad behavior to work?

[chapulina]

My preference would be to at least fix it in all of our libraries before merging this. I don't know if this could be considered an API/ABI break as it is depending on bad behavior to work?

It's bad practice to rely on indirect includes, so technically we don't have an obligation to keep the includes. It would be nice to add a note to the migration guide. We don't want to break our users knowingly though, and I'm pretty sure this will break people, so I'm in favor of targeting this at main for the public headers. It looks like the whole PR is targeting main anyway, so I no changes needed. +1 for fixing downstream libraries first.

[scpeters]

draft pull request to fix the missing includes in gz-sensors with homebrew and windows CI that builds against this branch: gazebosim/gz-sensors#242