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

Fix ordering and remove IGN_<LIB>_VER pattern #700

Merged
merged 3 commits into from
Apr 12, 2022
Merged

Fix ordering and remove IGN_<LIB>_VER pattern #700

merged 3 commits into from
Apr 12, 2022

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Apr 12, 2022 •
edited
Loading

Followups from: #699 , which introduced a bug where later replacements wouldn't get caught due to prior replacements.

Also introduces a rule to remove the IGN__VER N pattern:

    # Rule: IGN_<LIB>_VER <N> -> IGN_<LIB>_VER ${ignition-<lib><N>_VERSION_MAJOR}
    # Replace lines like: "set(IGN_PLUGIN_VER 2)"
    #               with: "set(IGN_PLUGIN_VER ${ignition-plugin3_VERSION_MAJOR})"
                                                              ^^^ <-- Bumped!

@methylDragon methylDragon mentioned this pull request Apr 12, 2022
Merged
@methylDragon methylDragon mentioned this pull request Apr 12, 2022
Open
5 tasks
@methylDragon
Copy link
Contributor Author

image

scpeters
scpeters reviewed Apr 12, 2022
View reviewed changes
release-repo-scripts/bump_dependency.bash Outdated
# with: "set(IGN_PLUGIN_VER 3)"
find . -type f ! -name 'Changelog.md' ! -name 'Migration.md' -print0 | xargs -0 sed -i "s@IGN_${DEP_LIB}_VER ${DEP_PREV_VER}@\UIGN_${DEP_LIB}_VER ${DEP_VER}@ig"
# with: "set(IGN_PLUGIN_VER ${ignition-plugin3_VERSION_MAJOR})"
find . -type f -name 'CMakeLists.txt' -print0 | xargs -0 sed -i "s@IGN_${DEP_LIB}_VER ${DEP_PREV_VER}@\UIGN_${DEP_LIB}_VER \L\$\{ignition-${DEP_LIB}${DEP_VER}_\UVERSION_MAJOR\}@ig"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this block and rely on updating the examples to use *_VERSION_MAJOR variables. I'll open an example PR to start

Copy link
Contributor

Choose a reason for hiding this comment

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

gazebosim/gz-rendering#607

Copy link
Contributor

Choose a reason for hiding this comment

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

gazebosim/gz-math#408

Copy link
Contributor

Choose a reason for hiding this comment

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

gazebosim/gz-common#339

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm, ordering does come an issue. Ok, it makes sense to remove the block but update the examples accordingly. I'll note it in the edge-case tracking issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

gazebosim/gz-physics#340

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to stop now; I think there may be a few more examples folders to check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

93a5107

Also updated note in issue: #689

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for helping with them! You got all of them except these: gazebosim/gz-sim#1442

🔥

This was referenced Apr 12, 2022
Merged
Merged
Merged
@methylDragon
Remove IGN_<LIB>_VER case instead
7d75fd1 
This pattern requires that the library is found before it is invoked. And in most cases, it'll be invoked first.
@methylDragon
Copy link
Contributor Author

@chapulina Just pinging you to let you know that I'm merging this, and this merge will revert the script so that it won't catch the IGN_LIB_VER case for the reasons stated here and in the issue #689

@methylDragon methylDragon merged commit 2dc1c0e into master Apr 12, 2022
@methylDragon methylDragon deleted the ch3/bump_dependency-remove-lib-ver-pattern branch April 12, 2022 18:39
@methylDragon methylDragon mentioned this pull request Apr 12, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters approved these changes

@j-rivero j-rivero Awaiting requested review from j-rivero j-rivero is a code owner

@chapulina chapulina Awaiting requested review from chapulina

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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