-
Notifications
You must be signed in to change notification settings - Fork 47
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
Digraphs on windows compiles to .dll and not to .so #177
Comments
@james-d-mitchell Any idea what we can do about this? |
@alex-konovalov @wilfwilson I don't really know what the problem is. Is this issue new, or did it always exist? I just had a look at the |
I haven't seen this before - prior to that, the build failed at an earlier stage as described in #138. The IO package compiles to |
Ah, so what happens if you simply rename |
And sorry for not being more help, I don't have access to a windows computer so can't experiment. |
In
In
|
So, it looks like in |
Yes, in the io
so the command
simply copies the dll to the same directory as usual, but renames it as
and so And is perhaps Digraphs putting the dll in the wrong place @james-d-mitchell? Note that |
(Sorry James I've repeated what you wrote, just wanted to look at it myself) |
I got access to a Windows machine and installed GAP 4.10.1 through the exe installer. I then renamed the
|
Note that the file really does seem to exist:
(i.e. it doesn't |
What happens when you do:
|
I can't tell you right now because that GAP installation broke somehow. I'll reinstall at some point. But everything I tried to do with IO seemed to work fine - and I think I tried that command (although maybe not fine, since I think I had already loaded IO at that point, but it didn't have the Digraphs error anyway). |
Looking at this again it looks like |
I feel like it used to exist. But it's probably not relevant here. |
So, the question is: is "digraphs" in SHOW_STAT() on windows? If it is, and the module is loaded by |
I'm sure it used to exist |
|
And then to answer your earlier question, immediately after my previous comment:
|
And when you do:
explicitly? |
Same again unfortunately:
|
And the file |
I'm looking at this. It seems to be looking for a file called "cygplanarity". Does the name ring any bells (maybe using a library called planarity?) |
Thanks @ChrisJefferson, yeah, I guess that |
which is contained in the |
I'll note that digraphs (unlike io) also has a
In particular there's a |
The list of places that GAP series for dlls is unfortunately not super useful. It will look in the \lib or \bin directory of your cygwin install, or in the '.libs' directory of the GAP install. It doesn't look anywhere inside the digraphs directory. |
@alex-konovalov This seems to work, I think 🙂 |
@ChrisJefferson Do you think all of the files from the Similarly, for semigroups, I can see that there is a |
You only need to copy dll files which are required by the dlls (renamed so) that you explictly load. |
Which tests don't pass? I just ran testinstall.tst from digraphs and it works fine (although I'm in a "full" cygwin install, so the problem could be how we package it) |
Thanks. |
Try |
|
Amusingly (well, the coincidence is amusing), this was just fixed by @embray's patch gap-system/gap#3335 |
both digraphs and semigroups tests pass on my computer with that patch applied and the two dlls (one for semigroups, one for digraphs) copied into GAP's .libs directory (and digraph's .dll renamed to .so). |
Nice |
@ChrisJefferson which was the semigroups dll that you copied to GAP's |
Is there anything I might be able to do to help here? If the Digraphs package uses autoconf and libtool it can be configured to output DLLs correctly. To be clear though, the runtime link path for DLLs is the same as your I think that in Sage's installation of GAP I've made sure that compiled GAP packages can be loaded properly in general, though I'm not sure if I've tried the digraphs package (nor do I fully understand why it would be different or special). |
We are using |
Technically, rather than rename the dll, digraph's init.g could be changed to load a .dll if it's in windows, and a .so if it's in linux, but I'm not convinced that makes things easier/clearer personally. Getting the second dll (cygplanarity-0.dll) to load is trickier, as that is autoloaded by windows as a requirement of the digraph dll, so we need to put it somewhere (like .libs) where windows will look for it. |
It's not actually tricky necessarily. The problem I'm struggling, which I don't understand, is how GAP searches for compiled packages e.g. on Linux. It seems with most packages if I run |
Also, confusingly, if I run |
I see. In the |
Nothing in gap had a working make install, and nothing is intended to be moved after it is built (other than copying the whole build tree) . A Gap just runs pkg binaries from where they are built. |
At the moment, when we distribute GAP on Cygwin we try to include the minimal pieces of Cygwin needed. I think that's just causing trouble, so in the next release I plan on including the complete minimal installation of Cygwin. This will make things easier as we can just copy these dlls into /lib. |
After thinking about this some more, I'm starting to realize that I think something GAP could use is a GAP-level interface to That way there would be less need for mucking around with rpaths, or making sure x file is in y directory. Otherwise I think the best alternative, at least on Windows, is for the package's init.g to set |
I should add, in the above case, it should also be easy to disable this in case a downstream packager (e.g. on Debian) would like to use the system's libplanarity instead, so long as it's compatible. Another alternative that might be still even simpler, since Digraphs is already vendoring libplanarity, would be to statically link it--or even just the parts of it needed by Digraphs--into the digraphs.so module. I have done the same, for example, in several Python extension modules. Though in this case it might be advisable to also prefix any libplanarity symbols. |
Thanks @embray, how do we statically link libplanarity into Digraphs.so? |
I will close this issue now. The fix to the issue is contained in |
I'm going to re-open this in the hope that Erik and I can find a "proper" solution, since Erik and I are in the same room currently. |
This works in GAP 4.10.2 release candidate:
so perhaps can be closed, unless you have further plans to improve something. |
I still can not provide a working version of digraphs (and hence semigroups, which needs it) for GAP 4.10.1. It compiles to
bin/i686-pc-cygwin-default32-kv3/digraphs.dll
while itsAvailabilityTest
checks fordigraphs.so
.Furthermore, this happens if I edit
AvailabilityTest
to check fordigraphs.dll
:The text was updated successfully, but these errors were encountered: