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.

Sign up for GitHub

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

Jump to bottom

ign -> gz Header Migration : ign-utils #53

Merged
merged 5 commits into from
Apr 28, 2022
Merged

ign -> gz Header Migration : ign-utils #53

merged 5 commits into from
Apr 28, 2022

Conversation

methylDragon
Copy link
Contributor

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Apr 26, 2022
@methylDragon methylDragon mentioned this pull request Apr 26, 2022
Closed
9 tasks
@methylDragon methylDragon force-pushed the header_migration branch 7 times, most recently from 7e0321c to 2c41344 Compare April 26, 2022 01:55
@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label Apr 26, 2022
chapulina
chapulina reviewed Apr 26, 2022
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM overall, I'm just concerned about maintaining the file's histories

cli/include/external-cli/ignition/utils/cli/Error.hpp Outdated
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2021 Open Source Robotics Foundation
* Copyright (C) 2022 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't change the copyright years, they represent the year the original file was first created

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be hard to automate it, I'll add a user warning to remind us to double check and unstage any unecessary copyright changes if there are any!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went down a deep rabbit hole. But now every copyright year that's changed will be programmatically unpatched!

gazebo-tooling/release-tools@971f5c6

Copy link
Contributor Author

@methylDragon methylDragon Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Automatically updated file (Error.hpp): https://github.com/ignitionrobotics/ign-utils/pull/53/files#diff-828663b8f5f0dcf98863f54c2325e72112e30bf629695007b6ae6accd9927550

cli/include/gz/utils/cli/GzFormatter.hpp
*
*/

#ifndef GZ_UTILS_CLI_IGNITION_FORMATTER_HPP_
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this file moved with git mv? Its blame only shows the commits in this PR:

https://github.com/ignitionrobotics/ign-utils/blame/2c41344a782b5fe894493f34162620757ac5fadd/cli/include/gz/utils/cli/GzFormatter.hpp

It's similar for some of the other moved files

Copy link
Member

Choose a reason for hiding this comment

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

yes, there's a problem with how the files are renamed. It should be a simple git mv command.

Compare e55aa67 with b2e1098

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think it's because to get the git diff to work in the script, I had to reset. I think I'll bypass that and force a commit, hopefully that gets the histories transferred!

Copy link
Contributor Author

@methylDragon methylDragon Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed!

gazebo-tooling/release-tools@0a46de1

The issue was the script was ignoring staged changes when checking git diff. I edited the commit function to be more flexible (gazebo-tooling/release-tools@daad715) to have it git diff --staged instead. (I was previously resetting/unstaging the changes which caused the blame to go awry.)

The blames should show up now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The blame looks good to me now!

Another thing we should check is whether squash-merging this PR would preserve that, or if we need to rebase-merge.

Copy link
Member

Choose a reason for hiding this comment

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

the header guard still has IGNITION

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Fixed!

gazebo-tooling/release-tools@5a44a5a

Copy link
Member

Choose a reason for hiding this comment

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

the header guard still needs to be fixed in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, looks like an edit to the regex I had on the script caused a reversion...

gazebo-tooling/release-tools@522cc9c
febac36

test/gtest/include/gtest/internal/gtest-internal.h Outdated
@@ -225,9 +225,9 @@ GTEST_API_ std::string GetBoolAssertionFailureMessage(
// The most-significant bit being the leftmost, an IEEE
// floating-point looks like
//
// sign_bit exponent_bits fraction_bits
// sgz_bit exponent_bits fraction_bits
Copy link
Contributor

Choose a reason for hiding this comment

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

oops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug squashed! gazebo-tooling/release-tools@0ab424b

@methylDragon methylDragon force-pushed the header_migration branch 5 times, most recently from 44e06f0 to 545ff65 Compare April 27, 2022 01:04
@scpeters
Copy link
Member

I would consider combining the changes from 26f1a89 and e3de70a into a single commit that adds the redirection headers

@methylDragon
Copy link
Contributor Author

methylDragon commented Apr 27, 2022 •
edited
Loading

I would consider combining the changes from 26f1a89 and e3de70a into a single commit that adds the redirection headers

Done!

The script now amends the previous commit (we need a previous commit to check for copyright changes so we can unpatch them.)
gazebo-tooling/release-tools@9c353f2

@methylDragon methylDragon mentioned this pull request Apr 27, 2022
Closed
@methylDragon
Manual: Update examples
e305fcb 
Signed-off-by: methylDragon <[email protected]>
@methylDragon methylDragon changed the title ign -> gz Header Migration ign -> gz Header Migration : ign-utils Apr 28, 2022
@methylDragon methylDragon merged commit e0067a0 into main Apr 28, 2022
@methylDragon methylDragon deleted the header_migration branch April 28, 2022 19:23
@methylDragon methylDragon restored the header_migration branch April 28, 2022 20:12
This was referenced Apr 28, 2022
Closed
Merged
scpeters
scpeters reviewed Apr 28, 2022
View reviewed changes
cli/include/external-cli/ignition/utils/cli/App.hpp
@@ -15,4 +15,4 @@
*
*/

#include <CLI/App.hpp>
#include <external-cli/gz/utils/cli/App.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

the external-cli/ should be removed from the #includes in all these files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f3c7fcd

scpeters
scpeters reviewed Apr 28, 2022
View reviewed changes
cli/include/vendored-cli/ignition/utils/cli/CLI.hpp
*
*/

#include <vendored-cli/gz/utils/cli/CLI.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

vendored-cli/ should be removed from all the #includes in these files

Copy link
Contributor Author

@methylDragon methylDragon Apr 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

f3c7fcd

scpeters
scpeters reviewed Apr 28, 2022
View reviewed changes
include/ignition/utils/utils.hh
@@ -0,0 +1,18 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

this redirect header points to nothing and it should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

7348ea2

scpeters
scpeters reviewed Apr 28, 2022
View reviewed changes
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

there is a redirect header missing for ignition/utils/detail/Export.hh

@methylDragon
Copy link
Contributor Author

there is a redirect header missing for ignition/utils/detail/Export.hh

3ecdccb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina left review comments

@scpeters scpeters scpeters approved these changes

@azeey azeey Awaiting requested review from azeey azeey is a code owner

Assignees
No one assigned
Labels
🌱 garden Ignition Garden ign to gz Renaming Ignition to Gazebo.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@methylDragon @scpeters @chapulina