-
Notifications
You must be signed in to change notification settings - Fork 126
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
Add a static windows build to CI #4037
Conversation
I can see something in here: https://github.com/actions/runner-images/blob/main/images/windows/Windows2022-Readme.md . It appears that the default image ships something called: Microsoft.VisualStudio.Component.VC.TestAdapterForGoogleTest that might be injecting the wrong settings so that we produce a shared lib instead of a static for gtest as it seems that we are doing. |
How about trying to update google test to a newer version, maybe this was resolved later on. BTW I strongly recommend not to use main but tag releases (v1.14.0 Latest) |
OK, but this PR forces external GTest to OFF, which seems to result in the desire behavior, which is GTest being build in thirdparty and the use of the same compiler settings as the rest of the project, yes? So I'm not sure how to apply the information above. I found a number of suggestions about modifying the flags specified in the visual studio project, bug of course this is CMake generating those, nothing that we've specified. We just did -DBUILD_SHARED=OFF. Could this be a cmake problem? |
hmm I think that they key issue is this: https://stackoverflow.com/questions/14714877/mismatch-detected-for-runtimelibrary. Basically we need to set the same https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html in both gtest and ADIOS2 |
Here is the excerpt from the CI of this PR:
|
I considered that too. The fly in the ointment there is that everything past v1.12 requires C++14 (which is more than the rest of ADIOS requires). Not a problem for a quick test, but potentially something to consider. I suppose we could also consider a win2019 static build to see if that was better, or at if it at least let us get to problems in ADIOS. |
Yeah, seeing this is why I tried forcing the use of the internal GTest. Didn't change anything though. Maybe find_package() is still picking up the external version despite us building one. |
that is not true, if I check the cmakecache.txt I see this:
|
hmm we are still having the issue. For some reason the flag CMAKE_MSVC_RUNTIME_LIBRARY is not being passed down to gtest. |
dd96df1
to
dfa8637
Compare
Now we only have errors with FFS |
Well, that's a start. I'll see what I can do from here. Thanks! |
Do you think that the remain errors are due to wrong dllexport directives in FFS? |
Yes, there are remains from pre-cmake build environments in ffs and dependent libraries. That it’s likely not just ffs and that I don’t have working windows CI in those libraries means the next steps will take a while.
…Sent from my iPhone
On Feb 19, 2024, at 4:53 PM, Vicente Bolea ***@***.***> wrote:
Do you think that the remain errors are due to wrong dllexport directives in FFS?
—
Reply to this email directly, view it on GitHub<#4037 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHF7GJEVPVNOX4VUNL3II3YUPCWBAVCNFSM6AAAAABDN2GUNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJTGE4TSNZSHE>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Does it make sense to force an export all for windows?
On Mon, Feb 19, 2024 at 5:02 PM Greg Eisenhauer ***@***.***>
wrote:
… Yes, there are remains from pre-cmake build environments in ffs and
dependent libraries. That it’s likely not just ffs and that I don’t have
working windows CI in those libraries means the next steps will take a
while.
Sent from my iPhone
On Feb 19, 2024, at 4:53 PM, Vicente Bolea ***@***.***> wrote:
Do you think that the remain errors are due to wrong dllexport directives
in FFS?
—
Reply to this email directly, view it on GitHub<
#4037 (comment)>,
or unsubscribe<
https://github.com/notifications/unsubscribe-auth/AAHF7GJEVPVNOX4VUNL3II3YUPCWBAVCNFSM6AAAAABDN2GUNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJTGE4TSNZSHE>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
—
Reply to this email directly, view it on GitHub
<#4037 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHFOFWPJONESBZ7GN63CE3YUPDWPAVCNFSM6AAAAABDN2GUNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJTGIYDONBRHE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
--
[image: logo shows large letter K with the word Kitware below it]
<http://www.kitware.com>
Vicente Bolea, MSc.
*Senior R&D Engineer*
|
Probably. I've just been stuck at CI until now. I was digging around trying to sort of things like GENERATE_EXPORT_HEADER are still necessary or recommended, but hadn't landed on any obviously correct examples. |
e0e1fbf
to
67a56fc
Compare
It appears that the issue was with the use of dllexport | import. The latest change seems to have a address it. https://github.com/ornladios/ADIOS2/actions/runs/7967848404/job/21751218798?pr=4037 IT also seems that this affects DILL too. |
e2818d8
to
4f14aa8
Compare
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.
We can't make direct changes inside thirdparty libraries (at least not without killing our ability to easily update them). You also have this in the dashboard_cache list in ci-win2-22-vs2-22-static-serial.cmake. I assume that wasn't sufficient? Can we do this in a file we own like thirdparty/GTest/CMakeLists.txt?
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.
@eisenhauer these are exploratory changes, once we find the right changes we can add them in their respective repos
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.
@eisenhauer these are exploratory changes, once we find the right changes we can add them in their respective repos
Understand of course, and that's my plan for the GTKorvo stuff, but I'm assuming that it's not so trivial for googletest...
Yeah, I knew the dllimport spec was the problem, but the question was how to fix it. It does look like changing to dllexport in source and outside helps. I can extend this to dill, atl and evpath as well (we're not pulling in evpath on Windows yet, but we need to for SST and remote stuff). No Windows CI on those packages, but the ADIOS CI can likely serve as an indirect proxy to try things out. Thanks! |
@vicentebolea Just to close the loop for the final status. In general, the declspec(dllimport) was the problem in the static build, but it wasn't necessary to replace them with dllexport. That is all taken care of by CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS. However, the one place we do still need a declspec is in dill where we are exporting not just code symbols, but also data symbols. Data symbols require a declspec(dllimport) to be imported by the VS compiler, but it must only appear in the declaration where the symbol is used (not where it's defined), and only when we are building shared and not static libraries. |
@eisenhauer sounds great, |
The EXPORT_ALL_SYMBOLS was actually already in use in all those GTkorvo projects, we just hadn't killed the declspec defines that were leftover from pre-CMake configuration. That they hadn't been killed both made the static build fail, but also happened to make the dynamic build work for the data symbols that dill needed to export). That messiness was probably why this hadn't been sorted before. In any case, I'm in the process of folding those changes into the upstream GTkorvo repos. When I'm done I'll probably kill this PR and re-submit something with a less complex history... Thanks for your help! |
Can you keep it in this PR, you can rebase/reset all the commits in a single one. In the future it will be easier to find the reason of this if this is done in a single PR 👼 |
The awkwardness it that I have to kill all the GTkorvo commits, because they really have to come in via the update script. I suppose I could reset all these commits away, kill the GTkorvo changes, redo them via update, etc. Just not keeping very much, but I guess it does keep the dialogue. |
1778dfa
to
aef1a7b
Compare
Code extracted from: https://github.com/GTkorvo/dill.git at commit 2bd038273c1083cdaddb9a402fff5db8dbb385f4 (master). Upstream Shortlog -----------------
# By dill Upstream * upstream-dill: dill 2024-02-21 (2bd03827)
Code extracted from: https://github.com/GTkorvo/ffs.git at commit cc3da1a07d5040a9d76e0fafe89b082ce6121145 (master). Upstream Shortlog -----------------
# By ffs Upstream * upstream-ffs: ffs 2024-02-21 (cc3da1a0)
Code extracted from: https://github.com/GTkorvo/EVPath.git at commit 6587d0f0d7a80dbc0afb56ae4aca9250fe366e57 (master). Upstream Shortlog -----------------
# By EVPath Upstream * upstream-EVPath: EVPath 2024-02-21 (6587d0f0)
aef1a7b
to
90f0640
Compare
The inability to do a rebase on any PR that has used the update.sh script in a thirdparty package is a nuisance... |
I think that the proper way would be to do a reset --soft (soft is the
default mode so just git reset)
…On Wed, Feb 21, 2024 at 2:06 PM Greg Eisenhauer ***@***.***> wrote:
The inability to do a rebase on any PR that has used the update.sh script
in a thirdparty package is a nuisance...
—
Reply to this email directly, view it on GitHub
<#4037 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHFOFXFILLKHQZMZ4VUNIDYUZATFAVCNFSM6AAAAABDN2GUNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJXG4YTSNRYGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
[image: logo shows large letter K with the word Kitware below it]
<http://www.kitware.com>
Vicente Bolea, MSc.
*Senior R&D Engineer*
|
Well, since rebase often simply doesn't work in that scenario (during the rebase commits to things like ADIOS2/thirdparty/ffs/ffs/CMakeLists.txt get applied to ADIOS2/CMakeLists.txt), you do have to have to do something else... |
@vicentebolea I believe that this is ready for review at this point. |
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.
Tiny comment, everything else great! Good job
Co-authored-by: Vicente Bolea <[email protected]>
Ok, removed that... |
- Upstream EVpath, FFS and dill. - Actually add the CI Co-authored-by: Vicente Bolea <[email protected]>
No description provided.