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

CSharp fixes #4893

Merged
merged 4 commits into from
May 30, 2023
Merged

CSharp fixes #4893

merged 4 commits into from
May 30, 2023

Conversation

jmarrec
Copy link
Collaborator

@jmarrec jmarrec commented May 23, 2023

Pull request overview

Fix 3.6.1 Csharp workflow that failed due to a Wunused-function warning. (likely because of updated compiler or something)

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 self-assigned this May 23, 2023
Comment on lines 34 to 35
with:
ref: master
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • TODO: REVERT

Comment on lines +54 to +58
add_library(csharp_warnings INTERFACE)
target_compile_options(csharp_warnings INTERFACE "$<${is_msvc_genex}:/wd4996>")
target_compile_options(csharp_warnings INTERFACE "$<${is_msvc_genex}:/bigobj>")
target_compile_options(csharp_warnings INTERFACE "$<${is_gnu_or_clang_genex}:-Wno-deprecated-declarations>")
target_compile_options(csharp_warnings INTERFACE "$<${is_gnu_or_clang_genex}:-Wno-unused-function>")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What?! Modern Cmake?! Yep!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anyways, the only new flag is -Wno-unused-function. But I'm using an interface library to avoid polluting the global namespace via add_definitions

Comment on lines +162 to +164
PRIVATE
openstudiolib
csharp_warnings
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And linking to that interface lib. And making the linkage PRIVATE for openstudiolib, another modern cmake practice which we already have in some places

Comment on lines +176 to +178
PRIVATE
openstudiolib
csharp_warnings
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and again for the rest of the csharp libs

@jmarrec
Copy link
Collaborator Author

jmarrec commented May 25, 2023

Dammmmmm I'm so dumb! I couldn't understand why the CMakeLists.txt changes fixed the build on my machine but not on Github Actions... I'm specifically building branch master so the CMakeLists.txt changes aren't in there...

@jmarrec
Copy link
Collaborator Author

jmarrec commented May 25, 2023

Ok, the 3.6.1 CSharp bindings can be found on https://github.com/NREL/OpenStudio/actions/runs/5077752228

@tijcolem can you upload them to https://www.nuget.org/packages/OpenStudio please?

@MingboPeng FYI, if you want to reupload the platform specific one

…g patched csharp/CMakeLists.txt"

This reverts commit 70cf714.
@jmarrec jmarrec requested a review from tijcolem May 25, 2023 13:28
@jmarrec jmarrec merged commit 78e0442 into develop May 30, 2023
@jmarrec jmarrec deleted the CSharp branch May 30, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant