-
Notifications
You must be signed in to change notification settings - Fork 192
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
Fix compiler issues #5160
Fix compiler issues #5160
Conversation
# ignore c++20 deprecation warnings generated by fmt | ||
add_definitions(-D_SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING) | ||
add_definitions(-D_SILENCE_ALL_MS_EXT_DEPRECATION_WARNINGS) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have an issue with these
@@ -25,7 +25,7 @@ add_library(openstudio_workflow | |||
Timer.cpp | |||
) | |||
|
|||
target_link_libraries(openstudio_workflow PRIVATE openstudiolib) | |||
target_link_libraries(openstudio_workflow PUBLIC openstudiolib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one looks slightly problematic to me, especially since it appears the issue is in the workflow_test
Severity Code Description Project File Line Suppression State Details
Error LNK2005 "public: virtual __cdecl openstudio::IdfObject::~IdfObject(void)" (??1IdfObject@openstudio@@UEAA@XZ) already defined in RunPreProcessMonthlyReports_GTest.obj openstudio_workflow_tests C:\repos\os\debug\src\workflow\openstudiolib.lib(openstudiolib.dll) 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the linker error that this change fixes, I don't know why you would want to PRIVATE link openstudiolib with openstudio_workflow, I don't think you can use openstudio_workflow without also linking openstudiolib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible that this might work instead, I'll try it out:
-target_link_libraries(openstudio_workflow PUBLIC openstudiolib)
+target_link_libraries(openstudio_workflow PRIVATE openstudiolib)
if(BUILD_TESTING)
set(openstudio_workflow_test_depends
+ openstudiolib
openstudio_workflow
- boost::boost # Maybe at some point replace with openstudiolib more simply
- fmt::fmt
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup that works, I can push if you prefer. However, that requires anyone who links openstudio_workflow to also link openstudiolib which is what the PUBLIC link option does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think anyone should link to openstudio_workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public is fine to me. You guys understand the implications.
2c58357
to
8e1e8cd
Compare
CI Results for 8e1e8cd:
|
@kbenne any opinion here? please |
8e1e8cd
to
7fa7410
Compare
Pull request overview
Fixes compiler issues I hit when attempting to build with Microsoft Visual Studio Community 2022 (64-bit) - Version 17.9.6
New definitions needed to overcome:
Public linking of openstudiolib in openstudio_workflow needed to overcome:
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.