-
Notifications
You must be signed in to change notification settings - Fork 6
fix: build failure on win32 #82
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: main
Are you sure you want to change the base?
Conversation
let name := nameToStaticLib "unicodeclib" | ||
buildStaticLib (pkg.sharedLibDir / name) oFiles | ||
|
||
extern_lib libunicodeclib := UnicodeCLib.fetch |
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.
Lake extern_lib
is apparently deprecated. I don't know what the best fix is here.
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 pinged Mac. I don't know when he may get back to me.
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.
Oh I didn't notice that when I made this PR, sorry.
I did a little bit of testing though, without extern_lib
, I have to turn off precompileModules
for it to build, but then we'll be losing the ability to run FFI functions in the interpreter so not ideal at all. Other than that I haven't had any luck yet getting this to build yet.
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 guess by Mac you mean the lake
maintainer -- Good call, because this might indeed be a problem of lake
itself: without extern_lib
, in the args that are fed to clang
, -lunicodeclib
and the corresponding path is missing, that's why ld
isn't able to find it, this is the direct cause of the issue. (Although it should be there because we've mentioned it in moreLinkObjs
I think.)
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.
No worries, this is a good suggestion that may work as a temporary fix. The alternative temporary fix is to pin UnicodeBasic to 782d3c3
This is top priority for me but I'm currently away from the office and I don't have access to a local Windows machine for testing. Please do let me know if your trials lead to a better solution.
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.
Is the Windows build somehow expecting a shared library (dll) too? (My Windows ignorance is showing 😿)
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 build fails when compiling TableLookup.lean
(It'll be compiled to a shared library .dll
as expected because we've specified precompileModules
), which depends on UnicodeCLib.
Because of that dependency naturally the linker'll start looking for symbols (such as unicode_prop_lookup
) defined in the CFFI. But it seems that lake
didn't provide ld
the correct arguments -lunicodeclib
and the path to that static lib and as a result ld
wouldn't be able to find it.
So I don't think that it's expecting a shared library. It's simply that the linker cannot find our unicodeclib.a
static lib on Windows.
on a separate note: I primarily use a mac so I also checked how this is done in macos (as well as linux), on these platform lake
tells clang
to use dynamic lookup (but not on win32) so although we compile our UnicodeCLib AFTER (as mentioned in the documentation of moreLinkLibs
) the compilation of other libraries it'll still be fine.
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.
Lake currently does not link moreLinkObjs
to module dynlibs due to concerns over duplicate symbols when linking multiple modules of the same library together. However, upon investigation, I think these concerns are unfounded and this is a bug in Lake.
Fixes #81