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

ign -> gz : Release Tools Migrate DSL #750

Merged
merged 18 commits into from
Aug 22, 2022
Merged

ign -> gz : Release Tools Migrate DSL #750

merged 18 commits into from
Aug 22, 2022

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Jun 24, 2022

Part of: #746

This PR attempts to migrate internal variables inside DSL files to whittle down the number of occurrences of ign/ignition.

@methylDragon methylDragon requested a review from j-rivero as a code owner June 24, 2022 01:01
@chapulina chapulina added the ign to gz Renaming Ignition to Gazebo. label Jun 24, 2022
jenkins-scripts/dsl/ignition.dsl Outdated Show resolved Hide resolved
jenkins-scripts/dsl/ignition.dsl Outdated Show resolved Hide resolved
jenkins-scripts/dsl/ignition_collection.dsl Outdated Show resolved Hide resolved
jenkins-scripts/dsl/ignition_collection.dsl Outdated Show resolved Hide resolved
@@ -347,8 +347,10 @@ for ((i = 0; i < "${#LIBRARIES[@]}"; i++)); do
cd ${TEMP_DIR}/homebrew-simulation
startFromCleanBranch ${BUMP_BRANCH} master

# NOTE(CH3): This won't interfere with $LIB=gzXXX so it's fine
# expand ign-* to ignition-*
FORMULA_BASE=${LIB/ign/ignition}
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll only use this script to bump gz now, from Gazebo-H onwards, so I think it's ok to remove all ign / ignition

@j-rivero
Copy link
Contributor

j-rivero commented Aug 3, 2022

Not sure what is the goal of this PR @methylDragon . I see changes in variable names inside the DSL but not sure if we are trying to deal with other script names in release-tools. Could you please complete the PR description with the goals of the changes, please?

@chapulina
Copy link
Contributor

I see changes in variable names inside the DSL but not sure if we are trying to deal with other script names in release-tools

We've been trying to change everything we can to reduce the clutter when grepping for ign. That includes internal variables that aren't external-facing. This makes it easier to find and maintain the places where ign is still needed, and modernizes the codebase so we don't have legacy ign references hanging around.

As for the external facing stuff, we need a plan to migrate the names and URLs that show up in http://build.osrfoundation.org. Job names like _dsl_ignition, https://build.osrfoundation.org/job/ignition_gazebo-ci-main-focal-amd64/, etc. We'll need to update jobs and badges in the source code when we make these changes.

@methylDragon methylDragon force-pushed the migrate_dsl branch 2 times, most recently from 8041e7d to 576ff9a Compare August 4, 2022 20:09
@methylDragon
Copy link
Contributor Author

methylDragon commented Aug 17, 2022

There was a change in #779 that I'd like to tweak

                  software_name="gz-${ign_sw}"
                  [[ ${ign_sw} == 'gazebo' ]] && software_name="gz-sim"
                  /bin/bash -xe "./scripts/jenkins-scripts/lib/project-default-devel-homebrew-amd64.bash" "\${software_name}"

We can leverage variable expansion, it's more concise, I think, if gazebo -> sim is the only case we're considering

${gz_sw/sim/gazebo}

I'm making this change during my rebase of this branch (since it's a conflict), do let me know if you'd rather I revert it, and I will

EDIT: I ended up going back to using software_name because I don't understand the syntax enough and I couldn't get the substitutions to work for some reason...

Find: (ign(ition)?[-_])\$\{gz([^/|-|\}]*)\}
Replace: $1${gz$3/sim/gazebo}

Signed-off-by: methylDragon <[email protected]>
@methylDragon
Copy link
Contributor Author

methylDragon commented Aug 17, 2022

See: e3a684b for more changes!

I spotted more internal vars!

@methylDragon methylDragon force-pushed the migrate_dsl branch 15 times, most recently from d54d275 to 9fef16d Compare August 17, 2022 22:07
@methylDragon methylDragon force-pushed the migrate_dsl branch 2 times, most recently from 2d0a33f to a7a46f5 Compare August 17, 2022 22:33
@methylDragon
Copy link
Contributor Author

methylDragon commented Aug 17, 2022

RFC: I think special attention needs to be paid to 8776491

I'm not sure of the syntax or if what I'm doing makes sense, but tests pass

Signed-off-by: methylDragon <[email protected]>
@j-rivero
Copy link
Contributor

Run a test on #795 the real XML changes displayed are:

diff -ur -I '.*<id>dashboard_portlet_.*</id>.*' /tmp/current_xml_configuration/ignition_citadel-ci-main-homebrew-amd64.xml /tmp/pr_xml_configuration/ignition_citadel-ci-main-homebrew-amd64.xml
--- /tmp/current_xml_configuration/ignition_citadel-ci-main-homebrew-amd64.xml	2022-08-19 16:46:00.323093363 +0000
+++ /tmp/pr_xml_configuration/ignition_citadel-ci-main-homebrew-amd64.xml	2022-08-19 16:44:48.238185025 +0000
@@ -64,7 +64,7 @@
         <hudson.tasks.Shell>
             <command>#!/bin/bash -xe
 
-/bin/bash -xe "./scripts/jenkins-scripts/lib/project-default-devel-homebrew-amd64.bash" "gz-citadel"
+/bin/bash -xe "./scripts/jenkins-scripts/lib/project-default-devel-homebrew-amd64.bash" "ignition-citadel"
 </command>
         </hudson.tasks.Shell>
     </builders>
diff -ur -I '.*<id>dashboard_portlet_.*</id>.*' /tmp/current_xml_configuration/ignition_fortress-ci-main-homebrew-amd64.xml /tmp/pr_xml_configuration/ignition_fortress-ci-main-homebrew-amd64.xml
--- /tmp/current_xml_configuration/ignition_fortress-ci-main-homebrew-amd64.xml	2022-08-19 16:46:00.879101044 +0000
+++ /tmp/pr_xml_configuration/ignition_fortress-ci-main-homebrew-amd64.xml	2022-08-19 16:44:48.810193675 +0000
@@ -64,7 +64,7 @@
         <hudson.tasks.Shell>
             <command>#!/bin/bash -xe
 
-/bin/bash -xe "./scripts/jenkins-scripts/lib/project-default-devel-homebrew-amd64.bash" "gz-fortress"
+/bin/bash -xe "./scripts/jenkins-scripts/lib/project-default-devel-homebrew-amd64.bash" "ignition-fortress"
 </command>
         </hudson.tasks.Shell>
     </builders>
diff -ur -I '.*<id>dashboard_portlet_.*</id>.*' /tmp/current_xml_configuration/ignition_garden-ci-main-homebrew-amd64.xml /tmp/pr_xml_configuration/ignition_garden-ci-main-homebrew-amd64.xml
--- /tmp/current_xml_configuration/ignition_garden-ci-main-homebrew-amd64.xml	2022-08-19 16:46:01.431108670 +0000
+++ /tmp/pr_xml_configuration/ignition_garden-ci-main-homebrew-amd64.xml	2022-08-19 16:44:49.366202084 +0000
@@ -64,7 +64,7 @@
         <hudson.tasks.Shell>
             <command>#!/bin/bash -xe
 
-/bin/bash -xe "./scripts/jenkins-scripts/lib/project-default-devel-homebrew-amd64.bash" "gz-garden"
+/bin/bash -xe "./scripts/jenkins-scripts/lib/project-default-devel-homebrew-amd64.bash" "ignition-garden"
 </command>
         </hudson.tasks.Shell>
     </builders>

@j-rivero
Copy link
Contributor

Run a test on #795 the real XML changes displayed are:

Should be fixed with 662c857

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.

I think we are good to go.

@Blast545
Copy link
Contributor

Minor comment: I see that two of the files changed are named jenkins-scripts/dsl/ignition.dsl and jenkins-scripts/dsl/ignition_collection.dsl, should these names be changed to gazebo as well?

Context question: after this is deployed, do we lose history of the buildfarm jobs with the old ignition names?

@Crola1702 FYI: Waiting for this to be merged before fixing the internal buildfarm tools.

@j-rivero
Copy link
Contributor

Minor comment: I see that two of the files changed are named jenkins-scripts/dsl/ignition.dsl and jenkins-scripts/dsl/ignition_collection.dsl, should these names be changed to gazebo as well?

We are not renaming files in this PR, just changing the internal variable names if I'm not wrong.

Context question: after this is deployed, do we lose history of the buildfarm jobs with the old ignition names?

AFAIK we are not changing any job name too. If you check the work in #795, the artifacts generated are the files changed and content changed in these files. Should be completely empty now.

@j-rivero j-rivero merged commit 891cf11 into master Aug 22, 2022
@j-rivero j-rivero deleted the migrate_dsl branch August 22, 2022 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ign to gz Renaming Ignition to Gazebo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants