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

Deprecate plugin in favor of gz-plugin #350

Merged
merged 4 commits into from
May 26, 2022
Merged

Deprecate plugin in favor of gz-plugin #350

merged 4 commits into from
May 26, 2022

Conversation

chapulina
Copy link
Contributor

Summary

  • 0e4a0f5: Move plugin headers back to ignition folder because users shouldn't migrate to gz/common/*Plugin*, but migrate to gz/plugin instead.
  • 7090764: Use the correct headers
  • e0f0e21: Add deprecation warnings

Test it

CI should pass without warnings.

My local cppcheck had some unknownMacro warnings, but that may be due to my version. I'll see what CI says and suppress those warnings as needed.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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.

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

@chapulina chapulina requested a review from mjcarroll as a code owner May 3, 2022 17:08
@github-actions github-actions bot added the 🌱 garden Ignition Garden label May 3, 2022
@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #350 (e0f0e21) into main (3aadb66) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #350   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files           1        1           
  Lines          48       48           
=======================================
  Hits           44       44           
  Misses          4        4           

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 3aadb66...e0f0e21. Read the comment docs.

@scpeters
Copy link
Member

scpeters commented May 3, 2022

I would recommend following the commit pattern from #348 in somewhat reverse order so we can more easily follow the steps:

  • first commit: remove the include/ignition/common/Plugin.* redirection headers
  • second commit: git mv the relevant include/gz headers back to include/ignition (and nothing else)
  • third commit: revert header guards and #include statements for the Plugin* files from 070a6c8

chapulina added a commit that referenced this pull request May 24, 2022
@chapulina chapulina force-pushed the chapulina/5/plugin branch from e0f0e21 to 64f0bd6 Compare May 24, 2022 20:02
@chapulina
Copy link
Contributor Author

@scpeters , I reorganized the commits as requested.

@methylDragon , it would be nice to get this in before #356

@mjcarroll , this is one that you could review if you're bored 😉

chapulina added a commit that referenced this pull request May 24, 2022
@chapulina chapulina force-pushed the chapulina/5/plugin branch from 64f0bd6 to 1e626ed Compare May 24, 2022 20:57
chapulina added a commit that referenced this pull request May 25, 2022
@chapulina chapulina force-pushed the chapulina/5/plugin branch from 1e626ed to 72799b0 Compare May 25, 2022 00:00
@methylDragon
Copy link
Contributor

methylDragon commented May 26, 2022

This is the first instance in the codebase of using XXX GZ_DEPRECATED(X) =

It might be something related to the ordering of the deprecation attribute?
The macro is supposed to resolve into __attribute__ ((__deprecated__)) in case it helps. It looks correct though... I don't know why windows is throwing a fit.

@methylDragon
Copy link
Contributor

methylDragon commented May 26, 2022

Traced it!

On windows it becomes:

#ifndef GZ_DEPRECATED
/// For ignition-common5-testing developers: Use this macro to indicate that a
/// function or class has been deprecated and should no longer be used. A
/// version should be specified to provide context to the user about when the
/// function became deprecated.
#define GZ_DEPRECATED(version) GZ_DEPRECATED_ALL_VERSIONS
#endif

then in the generated Export.hh detail header

#  define GZ_DEPRECATED_ALL_VERSIONS __declspec(deprecated)
#endif

https://build.osrfoundation.org/job/ign_common-pr-win/ws/ws/build/ignition-common5/include/gz/common/detail/Export.hh

Perhaps something's up with the __declspec()?

From some docs
"The __declspec keyword must prefix the declaration specification."

@methylDragon
Copy link
Contributor

methylDragon commented May 26, 2022

This thread says namespaces can't be __declspec()'ed (I presume it's the same for using declarations (though that's understandably MUCH harder to google for 😅 ))

Should we just selectively invoke the deprecated macro for everything but windows for the two using invocations?

@chapulina chapulina force-pushed the chapulina/5/plugin branch from 72799b0 to 66232e1 Compare May 26, 2022 20:30
Copy link
Contributor

@methylDragon methylDragon left a comment

Choose a reason for hiding this comment

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

👍 on green CI

@chapulina chapulina enabled auto-merge (rebase) May 26, 2022 20:52
@chapulina chapulina merged commit 6fa6fda into main May 26, 2022
@chapulina chapulina deleted the chapulina/5/plugin branch May 26, 2022 21:04
chapulina added a commit that referenced this pull request May 26, 2022
chapulina added a commit that referenced this pull request May 26, 2022
chapulina added a commit that referenced this pull request May 26, 2022
luca-della-vedova pushed a commit that referenced this pull request Jul 27, 2022
luca-della-vedova pushed a commit that referenced this pull request Jul 27, 2022
luca-della-vedova pushed a commit that referenced this pull request Jul 27, 2022
luca-della-vedova pushed a commit that referenced this pull request Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants