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

Properly set up CCACHE + fix doxygen config for versions up to 1.10.0 #5078

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

jmarrec
Copy link
Collaborator

@jmarrec jmarrec commented Jan 15, 2024

Pull request overview

This should work on windows. Haven't had time to confirm, but I did enable it successfully on OpenStudioApplication with the same change.

(One of) the old issue was that MSVC would fail to build RC files when ccache was picked up, and this won't.

RULE_LAUNCH_COMPILE is now flagged as an internal implementation detail, and it makes the compile of the RC file fail on windows.

For incremental rebuilds on the OSApp using github actions, for something that had very little change (or no change, only a workflow change for eg), I took the run time from 1h15 to about 15min (the build time itself went from 50 to 3min). So I'd say it's definitely worth trying to implement ccache on windows for Jenkins.

It's possible this won't work as is due to C# or something like this. Regardless, this is the correct way of enabling ccache on Unix as well, so this should be merged anyways

See the note on https://cmake.org/cmake/help/latest/prop_gbl/RULE_LAUNCH_COMPILE.html

Note This property is intended for internal use by ctest(1). Projects and developers should use the COMPILER_LAUNCHER target properties or the associated CMAKE_COMPILER_LAUNCHER variables instead.

Pull Request Author

  • Model API Changes / Additions
  • Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
  • Model API methods are tested (in src/model/test)
  • EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
  • If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
  • If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
  • Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
  • All new and existing tests passes
  • If methods have been deprecated, update rest of code to use the new methods

Labels:

  • If change to an IDD file, add the label IDDChange
  • If breaking existing API, add the label APIChange
  • If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • Code Style, strip trailing whitespace, etc.
  • All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
  • Labeling is ok
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified

@jmarrec jmarrec added Developer Issue Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Jan 15, 2024
@jmarrec jmarrec self-assigned this Jan 15, 2024
@jmarrec jmarrec changed the title Properly set up CCACHE Properly set up CCACHE + fix doxygen config for versions up to 1.10.0 Jan 16, 2024
The windows incremental runner was broken to due having 1.8.15 and us strongly requiring >=1.8.8,<1.8.9, so just fix the config.
@jmarrec
Copy link
Collaborator Author

jmarrec commented Jan 16, 2024

The windows incremental runner was broken to due having doxygen 1.8.15 and us strongly requiring >=1.8.8,<1.8.9, so just fix the config to make it work.

I have tested doxygen 1.10.0 locally and the search feature works now.

@ci-commercialbuildings
Copy link
Collaborator

ci-commercialbuildings commented Jan 16, 2024

CI Results for 72adf6f:

@jmarrec
Copy link
Collaborator Author

jmarrec commented Jan 16, 2024

Testing a full build on windows with ccache enabled by using this branch and a modified jenkins lib for the openstudio full build develop windows runner.

I can build properly with ccache enabled (though I kept running in the mt.exe manifest issue... we need to disable antivirus/windows defender scanning for the build dir): see https://ci.openstudio.net/blue/organizations/jenkins/openstudio-develop-full/detail/openstudio-develop-full/571/pipeline

Replaying a run that wipes the build dir and rebuilds, while the cache is hot (and no code changes) at https://ci.openstudio.net/blue/organizations/jenkins/openstudio-develop-full/detail/openstudio-develop-full/573/pipeline

Build took about 10min (cmake configure itself takes 9min!! That is horrendous).
The same Build would take approx 2h15 without ccache (it's been So long since we had a windows full build working on the first try that I found one from last may)

@jmarrec jmarrec requested a review from wenyikuang January 17, 2024 15:34
@jmarrec jmarrec merged commit 1ce97f5 into develop Jan 17, 2024
4 of 6 checks passed
@jmarrec jmarrec deleted the ccache-windows branch January 17, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Issue Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Found issue in doxygen generating documentation
2 participants