-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Export C++ library symbols for core methods #1298
Conversation
058cad9
to
c47045f
Compare
🎉 ... looks like this is working > dumpbin /exports cantera_shared.dll
Microsoft (R) COFF/PE Dumper Version 14.31.31104.0
Copyright (C) Microsoft Corporation. All rights reserved.
Dump of file cantera_shared.dll
File Type: DLL
Section contains the following exports for cantera_shared.dll
00000000 characteristics
FFFFFFFF time date stamp
0.00 version
1 ordinal base
391 number of functions
391 number of names
ordinal hint RVA name
[...] Main thing left is to add the |
@speth ... I am leaving this for the moment to get some feedback. While adding |
@@ -1019,6 +1019,9 @@ if env['coverage']: | |||
if env['toolchain'] == 'mingw': | |||
env.Append(LINKFLAGS=['-static-libgcc', '-static-libstdc++']) | |||
|
|||
if env["OS"] == "Windows": | |||
env.Append(CPPDEFINES=["CT_DLL_EXPORT"]) |
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 am assuming that this appends to CPPDEFINES
.
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 think you only want to set this while compiling the library sources, e.g. from the localenv
that's created in src/SConscript
. Setting it here will affect other cases like the tests, which is not what you want.
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 think that CT_DLL_EXPORT
needs to be defined when linking statically? (Also, builds with using the flag only on src/SConscript
won't compile, but I may be overlooking something)
I don't understand the details of what's happening here, but I have one thought: if these |
Yes - if this is pursued, then all external facing methods should receive this decoration. I was applying this conservatively for the moment as I'd like to get feedback on the general direction. |
Have you been able to actually use this for an example, when compiling on Windows? I tried compiling a pared-down version of the sample #include "cantera/base/Solution.h"
#include <iostream>
using namespace Cantera;
void demoprog()
{
auto sol = newSolution("h2o2.yaml", "ohmech");
}
int main(int argc, char** argv)
{
try {
std::cout << "running demoprog..." << std::endl;
std::cout.flush();
demoprog();
std::cout << "finished!" << std::endl;
} catch (std::exception& err) {
std::cout << "oops..." << std::endl;
std::cout << err.what() << std::endl;
}
} This compiles, but crashes before even getting to the bit of output. Even if I comment out the call to I know I had tried doing some work in this direction quite a while back, but abandoned it because there were just so many "gotchas" to doing this for even a moderately complex C++ API that wasn't designed with the quirks of Windows DLLs in mind. |
Not yet. I was mostly waiting on some initial feedback for go/no-go. My plan for testing is to get this to run for this demo (which works without issues on Linux). Your example is certainly easier! |
For me, I think the things that are important for the go/no-go decision on this will come down to how difficult it is to actually resolve the various challenges with getting this to work reliably and without too many limitations. Otherwise, I think it's easier to just suggest that users stick to using the static library on Windows for anything more than the C API. Potentially needing to add the I should mention, I also noticed that compiling the |
That’s good to know as it was one of my concerns that kept me from going deep into the weeds.
I would agree on that.
This is odd. I was under the impression that the test suite would cover that? All tests still pass … Regardless, based on this feedback I will pursue this idea as something that I would like to include in 3.0, but not necessarily with a high priority. |
dd5d963
to
eef3df8
Compare
Lacking bandwidth/interest to pursue this further. |
Reopening with a help wanted label. While I won’t be able to pursue this myself, it is still documenting some attempts to introduce this feature. |
I wanted to take a new look at this, since it seems that generating a more complete shared library is necessary to resolve Cantera/conda-recipes#40 cleanly. Using this PR, I tried compiling the demo program above with Visual Studio 2022, and learned a few things. First, on why the demo program was crashing without generating output: one cause of this is not having Second, when compiling the demo program with VS2022, I get the error:
If we look at the relevant lines of template <typename... Args>
CT_API CanteraError(const std::string& procedure, const std::string& msg,
const Args&... args)
: procedure_(procedure)
{
if (sizeof...(args) == 0) {
msg_ = msg;
} else {
msg_ = fmt::format(msg, args...);
}
} I think there are two possible fixes to this. One is to separate the declaration and implementation (both in the header), with the latter toggled with an // Inside CanteraError class
template <typename... Args>
CT_API CanteraError(const std::string& procedure, const std::string& msg,
const Args&... args);
// Outside CanteraError class, but still in ctexceptions.h
#ifdef CT_DLL_EXPORT
template <typename... Args>
CanteraError::CanteraError(const std::string& procedure, const std::string& msg,
const Args&... args)
: procedure_(procedure)
{
if (sizeof...(args) == 0) {
msg_ = msg;
} else {
msg_ = fmt::format(msg, args...);
}
}
#endif This unfortunately seems like it will require a lot of ugly boilerplate throughout the codebase. The other option is to simply remove the Finally, I also noticed that I am able to call some methods that are not marked with void demoprog()
{
auto sol = newSolution("h2o2.yaml", "ohmech");
std::cout << sol->thermo()->report() << std::endl;
std::cout << "phi = " << sol->thermo()->electricPotential() << std::endl;
} despite the fact that |
Replaces declarations of CANTERA_USE_INTERNAL
Use standard CT_API preprocessor for funcWrapper.h
Co-authored-by: Ray Speth <speth@mit.edu>
Does not work with Visual Studio 2022
I pushed a rebased version of this to my fork (https://github.com/speth/cantera/tree/export-symbols) which resolves the noted merge conflicts. I also added the fix to remove |
@speth ... depending on where this is going, I can do a hard reset on my repo to your branch (edit: I just reset my branch to yours) as long as there aren't major revisions necessary; if there is substantial additional development, feel free to close this PR and start a new one. |
eef3df8
to
45323b6
Compare
Superseded by #1429 |
Changes proposed in this pull request
CANTERA_USE_INTERNAL
byCT_DLL_EXPORT
CT_API
and use it for selected C++ objectsIf applicable, fill in the issue number this pull request is fixing
Closes Cantera/enhancements#140
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build
&scons test
) and unit tests address code coverage