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

Merge sdf9 ➡️ sdf12 #1346

Merged
merged 2 commits into from
Nov 14, 2023
Merged

Merge sdf9 ➡️ sdf12 #1346

merged 2 commits into from
Nov 14, 2023

Conversation

azeey
Copy link
Collaborator

@azeey azeey commented Nov 14, 2023

➡️ Forward port

Port sdf9 ➡️ sdf12

Branch comparison: sdf12...sdf9

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

* Use on `push` only on stable branches to avoid duplicate runs
* Update project automation

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey requested a review from scpeters as a code owner November 14, 2023 03:08
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Nov 14, 2023
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1a484c0) 92.14% compared to head (7a9dad5) 92.14%.

❗ Current head 7a9dad5 differs from pull request most recent head 7ab0cb8. Consider uploading reports for the commit 7ab0cb8 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            sdf12    #1346   +/-   ##
=======================================
  Coverage   92.14%   92.14%           
=======================================
  Files          79       79           
  Lines       13126    13126           
=======================================
  Hits        12095    12095           
  Misses       1031     1031           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@azeey
Copy link
Collaborator Author

azeey commented Nov 14, 2023

@j-rivero There's a new cmake warning in the homebrew build, I think because psutil is not being installed anymore.

@azeey
Copy link
Collaborator Author

azeey commented Nov 14, 2023

I'll go ahead and merge since this isn't the cause for the homebrew warning on Jenkins.

@azeey azeey merged commit f5b5333 into sdf12 Nov 14, 2023
8 checks passed
@azeey azeey deleted the azeey/9_to_12 branch November 14, 2023 20:25
@scpeters
Copy link
Member

@j-rivero There's a new cmake warning in the homebrew build, I think because psutil is not being installed anymore.

the need to install this pip package has historically been encoded in sdformat-default-devel-homebrew-amd64.bash, but I believe this script is no longer being called as part of #1010

would it be better to add a .github/ci/macos_requirements.txt file with needed pip packages similar to the .github/ci/packages.apt file and then update the project-default-devel-homebrew-amd64.bash script to look for that file and install those packages?

@j-rivero
Copy link
Contributor

@j-rivero There's a new cmake warning in the homebrew build, I think because psutil is not being installed anymore.

Ouch, I think that this is a regression introduced while migrating to gz-collections.yaml. Should be fixed by gazebo-tooling/release-tools#1072

@azeey
Copy link
Collaborator Author

azeey commented Nov 15, 2023

@j-rivero There's a new cmake warning in the homebrew build, I think because psutil is not being installed anymore.

Ouch, I think that this is a regression introduced while migrating to gz-collections.yaml. Should be fixed by gazebo-tooling/release-tools#1072

That PR seems to be removing cmake warnings from Jenkins. I thought having the warning was useful since it notified us that psutil was not installed. My understanding is that before the recent gz-collection changes, it used to be installed, and now it's not, which is what's causing the warnings.

@j-rivero
Copy link
Contributor

My understanding is that before the recent gz-collection changes, it used to be installed, and now it's not, which is what's causing the warnings.

The gz-collection,yaml changes affects the way of generating the Jenkins jobs and how that Jenkins job configuration is being generated. It should not affect the way of provisioning the CI nodes (in this case Mac nodes for Brew) . The gz-collection.yaml migration should not impact (and hardly can do it) the software/dependencies installed.

I thought having the warning was useful since it notified us that psutil was not installed.

Might have been useful in this case if you think that having psutil on Mac is useful. Until we can explicitly enforce using gz-cmake what dependencies we really require to be in a build it is hard to known which optional support we expect to find or which one we don't have interest into or it is not viable, specially on Windows/Mac.

@azeey
Copy link
Collaborator Author

azeey commented Nov 15, 2023

I have no strong opinion on whether psutil is useful on Mac or not. I was just pointing out that this is changing behavior from what we were doing since the dependency was explicitly included in https://github.com/gazebo-tooling/release-tools/blob/37e22107c76c49e2575f799a035bb60243aa276e/jenkins-scripts/sdformat-default-devel-homebrew-amd64.bash#L7. If we no longer need to run the psutil tests on Mac, that's okay with me.

@j-rivero
Copy link
Contributor

I was just pointing out that this is changing behavior from what we were doing since the dependency was explicitly included in https://github.com/gazebo-tooling/release-tools/blob/37e22107c76c49e2575f799a035bb60243aa276e/jenkins-scripts/sdformat-default-devel-homebrew-amd64.bash#L7. If we no longer need to run the psutil tests on Mac, that's okay with me.

Understood. It is a regression since we are provisioning software in the CI scripts. I'll fix that.

@scpeters
Copy link
Member

scpeters commented Nov 15, 2023

would it be better to add a .github/ci/macos_requirements.txt file with needed pip packages similar to the .github/ci/packages.apt file and then update the project-default-devel-homebrew-amd64.bash script to look for that file and install those packages?

this is another option for where to specify the list of needed packages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants