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

Split gzutils into functional pieces #344

Merged
merged 6 commits into from
May 8, 2023
Merged

Conversation

mjcarroll
Copy link
Contributor

GzUtils has gotten unmaintainably large. This splits the functions into separate files.

@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Apr 19, 2023
@mjcarroll mjcarroll force-pushed the mjcarroll/split_gzutils branch from fe2bcab to f8e454a Compare April 19, 2023 14:51
@j-rivero
Copy link
Contributor

GzUtils has gotten unmaintainably large. This splits the functions into separate files.

+1 . Side effects of merging this:

  • it's going to be hard to do forward/backward ports from/to the gz-cmake2 branch. Is there anything stopping us of sending this to the gz-cmake2 branch?
  • The current open PRs will need to rebase and probably adapt new code to this change.

@mjcarroll
Copy link
Contributor Author

it's going to be hard to do forward/backward ports from/to the gz-cmake2 branch. Is there anything stopping us of sending this to the gz-cmake2 branch?

Hmm, good call. I don't think there is anything stopping us from doing that here. I may re-open this against cmake2, we can land it with Ign file names and then apply the renaming?

The current open PRs will need to rebase and probably adapt new code to this change.

I think git should track the split/renaming without too much issue, but let me try it on one PR first.

@j-rivero
Copy link
Contributor

it's going to be hard to do forward/backward ports from/to the gz-cmake2 branch. Is there anything stopping us of sending this to the gz-cmake2 branch?

Hmm, good call. I don't think there is anything stopping us from doing that here. I may re-open this against cmake2, we can land it with Ign file names and then apply the renaming?

Yep, a bit of extra-work sorry but probably will make things easier for the future and reorganize the things on gz-cmake2 as well.

The current open PRs will need to rebase and probably adapt new code to this change.

I think git should track the split/renaming without too much issue, but let me try it on one PR first.

Thanks, anyway we can work on solving the problems that appear.

@scpeters
Copy link
Member

it's going to be hard to do forward/backward ports from/to the gz-cmake2 branch. Is there anything stopping us of sending this to the gz-cmake2 branch?

Hmm, good call. I don't think there is anything stopping us from doing that here. I may re-open this against cmake2, we can land it with Ign file names and then apply the renaming?

The current open PRs will need to rebase and probably adapt new code to this change.

I think git should track the split/renaming without too much issue, but let me try it on one PR first.

I think it would be easier to resolve the conflicts when merging forward if the change has been applied separately to each branch. I would be fine with merging to gz-cmake3 first and then backporting to ign-cmake2. There's already a conflict on gz-cmake3 though

@scpeters
Copy link
Member

scpeters commented May 5, 2023

conflicts

@j-rivero
Copy link
Contributor

j-rivero commented May 5, 2023

conflicts

I've tried to resolve them. Mostly applied #326 in 7ceb5f6 and #334 in cbf5249

Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Good for me. @mjcarroll I leave it open until you or @scpeters give a last view on this.

@scpeters
Copy link
Member

scpeters commented May 6, 2023

Good for me. @mjcarroll I leave it open until you or @scpeters give a last view on this.

I double-checked the changes using meld and noticed a few small differences: see #349. I'll be ready to approve if we merge that into this branch.

@j-rivero
Copy link
Contributor

j-rivero commented May 8, 2023

I double-checked the changes using meld and noticed a few small differences: see #349. I'll be ready to approve if we merge that into this branch.

Good catch, merged!

@mjcarroll
Copy link
Contributor Author

Thanks Steve, I think we are all good here!

@mjcarroll mjcarroll merged commit 88e7dc5 into gz-cmake3 May 8, 2023
@mjcarroll mjcarroll deleted the mjcarroll/split_gzutils branch May 8, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants