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

CI fixes: Mac runner upgrade and Doxygen installations #3921

Merged
merged 24 commits into from
Oct 4, 2024

Conversation

nickbianco
Copy link
Member

@nickbianco nickbianco commented Sep 30, 2024

Brief summary of changes

Upgrades all Mac runners to macOS 14 now that macOS 12 will be officially deprecated soon (already receiving warnings in GitHub Actions). Attempts to fix the Doxygen install issue on Windows; there is a comment suggesting that a choco install was previously unreliable, but giving it a try again here.

Testing I've completed

[perf-win]

Looking for feedback on...

CHANGELOG.md (choose one)

  • no need to update because...CI fixes.

Performance analysis

Platform: Windows, self-hosted runner

Test Name lhs [secs] stderr [secs] rhs [secs] stderr [secs] Speedup
Arm26 0.34 0.00 0.35 0.00 0.98
ellipsoid_wrap 4.09 0.00 4.06 0.00 1.01
ellipsoid_wrap_function_based_paths 3.39 0.00 3.39 0.00 1.00
Gait2354 0.43 0.00 0.42 0.00 1.01
MocoSlidingMass 1.49 0.01 1.43 0.00 1.04
MocoSquatToStand 13.54 0.06 13.49 0.04 1.00
passive_dynamic 5.62 0.01 5.59 0.01 1.00
passive_dynamic_noanalysis 3.37 0.00 3.33 0.00 1.01
RajagopalModel 7.88 0.01 7.90 0.00 1.00
ToyDropLanding 12.36 0.04 12.33 0.01 1.00
ToyDropLanding_fbp_stepwisereg 11.09 0.01 11.08 0.01 1.00
ToyDropLanding_function_based_paths 11.24 0.02 11.28 0.03 1.00
ToyDropLanding_nomuscles 0.56 0.00 0.57 0.00 0.98

This change is Reviewable

@nickbianco nickbianco requested a review from aymanhab September 30, 2024 23:20
@aymanhab aymanhab closed this Oct 1, 2024
@aymanhab
Copy link
Member

aymanhab commented Oct 1, 2024

ci changes LGTM 👍 other unrelated changes, however, snuck in, please remove if these need separate review. Feel free to merge after addressing..

@aymanhab aymanhab reopened this Oct 1, 2024
Copy link
Member

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

Would be good to limit this to ci, unless you feel the other changes are relevant to this PR

@nickbianco
Copy link
Member Author

Would be good to limit this to ci, unless you feel the other changes are relevant to this PR

The CI failures on Mac seems to be related to updating the runner to macOS 14. Some of the failures are due to overly strict floating point comparisons, so not surprising that these failures are cropping up. I'm trying to use Catch's recommend floating point comparison tools to address them. I'm not sure why testOutputReporter is failing yet though.

@nickbianco
Copy link
Member Author

Note: could improve Doxygen install based on simbody/simbody#801.

@nickbianco nickbianco requested a review from aymanhab October 3, 2024 00:09
@nickbianco
Copy link
Member Author

@aymanhab asking for another review here since I've made several changes. Finally patched all the testing failures, some of them were hidden because we were using non-Catch2 macros (e.g., testOutputReporter). I made a few other minor changes, including removing unnecessary code in testOutputReporter, upgrading the GitHub Actions checkout module, and using ubuntu-latest rather than macos-12 to run the perf testing jobs.

Copy link
Member

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @nickbianco I'd just spot check Doxygen to see if the result is usable or if we need to upgrade or configuration files
:lgtm:

Reviewed 2 of 4 files at r3, 1 of 1 files at r4.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @nickbianco)


.github/workflows/continuous_integration.yml line 30 at r4 (raw file):

      run: |
        (New-Object System.Net.WebClient).DownloadFile("https://github.com/doxygen/doxygen/releases/download/Release_1_12_0/doxygen-1.12.0.windows.x64.bin.zip", "doxygen.zip")
        7z x $env:GITHUB_WORKSPACE/doxygen.zip -odoxygen

We were sticking with 1.8.14 on windows because later versions were problematic and produced unexpected docs. Would be good to spot check the layout/colors/etc. locally

@nickbianco
Copy link
Member Author

Tried using SimTK::Eps in some places in testDeGrooteFregly2016Muscle, but those tests failed again, so reverting back to manual tolerances (seems like SimTK::Eps is getting simplified to 0.0 on macos-14).

I will take a look at the docs generated with the new version of Doxygen.

@nickbianco
Copy link
Member Author

It looks like the latest Doxygen version does produce some odd formatting of the docs:

image

I'll try to revert to an earlier version and see if it's better.

@nickbianco
Copy link
Member Author

Actually, the image I posted above is from the Mac CI builds, which already had been installing the latest version of Doxygen (1.12) via Homebrew. So the changes in this PR actually bring the Windows and Mac builds in sync on the Doxygen version.

If it's okay with you @aymanhab, I think I'll open a separate issue specific issues with using the latest Doxygen. That way we can get CI up and running in the meantime.

@nickbianco
Copy link
Member Author

(And we can always build the docs locally with whatever version of Doxgyen we need for releases, etc.)

@aymanhab
Copy link
Member

aymanhab commented Oct 4, 2024

Sure, let's merge if you're happy with everything else so we have successful builds. It may make sense to build Doxygen on windows only if unused on other platforms but that's to discuss separately on the Doxygen issue.

@nickbianco
Copy link
Member Author

Opened the Doxygen issue here: #3930.

Merging this one. Thanks @aymanhab!

@nickbianco nickbianco merged commit f874170 into main Oct 4, 2024
11 of 12 checks passed
@nickbianco nickbianco deleted the ci_fixes_09302024 branch October 4, 2024 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants