-
Notifications
You must be signed in to change notification settings - Fork 52
Windows DLL support #107
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
base: master
Are you sure you want to change the base?
Windows DLL support #107
Conversation
|
@eteq @mhvk @sergiopasra |
mhvk
left a comment
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.
@laheller - thanks for the PR. As is, this will unfortunately not work. ERFA is essentially a copy of SOFA with a different license, and we want to keep differences between the two to an absolute minimum -- see the README for the project as a whole. This means any of the regular files cannot be changed, including the erfa.h header (or it would have to be done in the copy process described in RELEASE).
Separately, whatever is added should be covered by CI, so it should be tested that *dll are produced that pass the tests.
Finally, as I work on linux not windows, I'm a bit confused: we already build for windows using meson, why do we need a different method?
Hi @mhvk If I understand correctly, the Windows/meson build generates a static library ( I checked the things little bit and I think I can rework this PR so that the original C header files are Rework in progress... BR, Ladislav |
|
@laheller - again, I know little about windows, but a quick google suggests it is possible to build a I'm not sure it isn't simply a bug that we don't do so already, since we do build a shared library on linux. |
@mhvk BR, Ladislav |
|
@laheller - see 66aefd5 for the commit that caused only the static library to be built. Let me ping @eli-schwartz since he put it there and mentioned in the commit message that
I guess we need to make sure both libraries get tested somehow. (I do continue to feel out of my league here, since I don't use windows. Ping @astrofrog, who may have more experience.) |
I am not a Windows user either. Unfortunately I don't feel out of my league at all ;) since I maintain a cross platform build system (Meson) that has to support Windows and therefore I know way too much, all things considered, about Windows. So: one of the original rationales for adding Meson support to erfa was in fact to support Windows (annoying to do if the build system is Autotools). Introducing Meson provided that for free, as a side effect of reducing the number of lines of code (by dropping Autotools) and making the build faster and more ergonomic. An inherent difference of windows is as this PR correctly noted, that:
A library with zero symbols is useless and counterproductive, obviously. That means that on Windows, you must do extra effort to produce shared libraries; on Unix, shared libraries simply work by default, but expose "everything and the whole world" in your ABI. ABI control is possible on all platforms.
Doing explicit ABI control is good, regardless of platform, since GCC can generate more efficient code if it knows a function will not be exposed as a symbol. Visibility attributes only -- the linker version script approach occurs too late to improve codegen, all it does is hide symbols from the symbol table (that does still make the runtime loader faster). |
|
I STRONGLY advise to not accept a visual studio solution checked into git. This is 1500 lines worth of mostly inscrutable xml, and meson can simply generate it for you (it is an optional alternative to ninja files). Of course, I'm biased since I'm an upstream meson developer. ;) But as a biased person I know very well that I'm also right. :D It's simple to wire up a def file to meson, simply pass the vs_module_defs kwarg to the library. I think you want to do that anyway, to avoid having something that works in visual studio but is broken in meson! |
The meson CI will already test the static library, and you can add a CI matrix to build on windows with default_library=shared and it will test the DLL version as well (because the test will try to link a test executable to the DLL and then run it). Obviously that will only work once ABI control for windows is implemented. |
|
@eli-schwartz @mhvk |
|
@eli-schwartz - thanks so much! It sounds with meson a solution is quite possible. @laheller - ping me in when it becomes worthwhile to run CI. As @eli-schwartz suggested, we'd like not to add inscrutable files (or rather, auto-generate them as needed). But I think you're already going towards that! |
7417886 to
92a59c6
Compare
|
@eli-schwartz @mhvk Now I can focus on my original goal, to create a C# .NET wrapper around the ERFA C-language functions. It will be a new repo that will reference this one. Or... what about to add add C# language support here? What do you think? |
mhvk
left a comment
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.
Thanks! I'll let CI run, to see if tests pass.
Still some comments, though: this PR adds two files which will have to be kept up to date whenever a new release is made. Ideally, there is just one source of truth.
My tendency would be to keep this repo as simple as it can be, and have wrappers elsewhere (like we have |
e433d00 to
05f54bc
Compare
First, I will create the C# wrapper as a separate repo, including some test code based on BTW this is how a C# wrapper looks like, in the below example it references the const string LibName = "erfa.dll"; // if the shared library is available, the .NET runtime loads it automatically
/// <summary>
/// Function documentation is available at:
/// <see href="https://www.iausofa.org/s/sofa_vm_c.pdf#page=23" />
/// </summary>
[DllImport(LibName, EntryPoint = "eraA2af", CharSet = CharSet.Ansi)]
public static extern void eraA2af(int ndp, double angle, StringBuilder sign, [Out] int[] idmsf);I think, now this PR is ready to merge. BR, Ladislav |
mhvk
left a comment
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.
Yes, this is getting nice and simple, and very close to being ready to merge. A few more questions in-line (none critical).
| @@ -0,0 +1,256 @@ | |||
| VERSION 0.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.
What is this version? Just of the .def file?
As I mentioned, ideally this file is generated automatically. How did you create it? Might that be transferable? (If not, we can do this as follow-up.)
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 is just the DLL versioning, a kind of metadata that we can add to the final DLL during the build and it can be displayed using the dumpbin /HEADERS erfa.dll command and has nothing to do with the erfa/sofa versions.
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.
shouldn't it match the soversion then?
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.
@avalentino
If the soversion is defined somewhere, then yes.
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 is defined in meson.build (and configure.ac, but let's not worry about that...):
Lines 10 to 20 in 1d9738b
| # The historic versions use libtool-compatible versioning. | |
| # This uses some gnarly math to define ABI versions, which we replicate here. | |
| # The general formula is: | |
| # libtool: C:R:A | |
| # -soname: (C - A).A.R | |
| libtool_version = [9, 1, 8] | |
| soversion = '@0@.@1@.@2@'.format( | |
| libtool_version[0] - libtool_version[2], | |
| libtool_version[2], | |
| libtool_version[1], | |
| ) |
I wonder if this file can be generated automatically by meson (maybe with help of a script)...
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.
The question is what are you going to parse and what rules to apply. :) The header file? All symbols that have the word "erfa" in them?
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.
@eli-schwartz - in our case I think ls src/*.c basically gives us what we need (strip .c, capitalize first character, and prefix with era). I think that should not be too difficult to do from meson (if just by calling out to some few-line script), but will admit my meson-fu is quite lacking (still too used to make...).
But I was asking mostly since @laheller presumably made this file one way or another, so perhaps he already used some handy one-liner. If not, I'll have a look myself, and push either to this PR or do a follow-up. If it turns out to be less easy than I think, I can always adjust the scripts that do the translation of sofa to erfa, at https://github.com/liberfa/erfa-fetch, to also create this file.
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.
@mhvk
If you ask, how I made the DEF file:
I used the .NET version of the libclang library to parse the erfa.h and erfaextra.h headers to easily get the C function names (+ the whole signature needed for the C# wrapper).
MNT: upgrade all github actions and pin them to hashes SEC: disable all default permissions at workflow level Add Windows DLL support. Add Windows DLL support. Add Windows DLL support. Add DEF file for Windows shared library Delete Visual Studio project files Setup CI & meson to build Windows DLL Fix erfa.def Add Windows DLL support. Cleanup Add Windows DLL support. Create shared library only for Windows Test CI build using LIBRARY = erfa.dll Fix erfa.def Add Windows DLL support. Fix erfa.def Add Windows DLL support. Fix src/erfaversion.c Add Windows DLL support.
All resolved. |
|
The following python script reproduces |
|
I'm still somewhat confused about what we need to do with VERSION, since DLL seems to do it differently from libtool. https://gnuwin32.sourceforge.net/versioning.html suggests using the first libtool number (current-age) as part of the file name. |
|
@mhvk |
|
@laheller - yes, I'd like to auto-generate the .def, but I think the script that I posted above will do it. For me, the only issue outstanding is what should be the version for the DLL file (see previous message). |
@mhvk |
On Windows and Visual Studio 2022 (or newer), one wants to easily build ERFA and produce a library (erfa.dll) that can be called from any language that supports language bindings.