Skip to content
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

Fix a problem with kernel extensions on macOS #5235

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Dec 1, 2022

On macOS, kernel extensions built against non-installed GAP could not be loaded into an executable built using libgap, such as tst/testlibgap/basic. Resolve this by building kernel extensions again with flat namespace (thus matching the behavior of the Linux dynamic linker more closely), as we used to when we were still using libtool for kernel extensions

This problem was only relevant for non-installed GAP builds. For GAP installed via make install and kernel extensions built against that, all was and is fine.

Resolves #5232

(To be clear: in general kernel extensions are highly specific to the GAP they were built again, using one built against a non-installed GAP in a different executable built against libgap is not a good idea in general; but since it was trivial to get it working in this case, I did it; but on the long run I expect this to be problematic for other reasons, so I'd advise against it)

On macOS, kernel extensions built against non-installed GAP could
not be loaded into an executable built using libgap, such as
`tst/testlibgap/basic`. Resolve this by building kernel extensions
again with flat namespace (thus matching the behavior of the Linux
dynamic linker more closely), as we used to when we were still
using libtool for kernel extensions

This problem was only relevant for non-installed GAP builds.
For GAP installed via `make install` and kernel extensions built
against that, all was and is fine.
@dimpase
Copy link
Member

dimpase commented Dec 1, 2022

This problem was only relevant for non-installed GAP builds.

This seem to be the case only for some values of non-installed.
At least, on https://trac.sagemath.org/ticket/34391 we've run into this after installing extra packages into
GAP already installed with make install.

@dimpase
Copy link
Member

dimpase commented Dec 1, 2022

ok, good, this allows the test proposed in #5234 to pass.

@fingolfin
Copy link
Member Author

Was the io.so in that Trac report really built against the installed GAP? I see that in one of your patches, you build io against the non-installed GAP

That said, the error message references /Users/dima/software/sage/local/lib/gap/pkg/io/bin/x86_64-apple-darwin22-default64-kv8/io.so so perhaps that's a different io. But you said on gap-packages/PackageManager#105

we don't run in BuildPackages.sh in Sage, we have our own contraption to deal with

So, how exactly do you build packages?

To be sure, I just did a test with a fresh GAP 4.12.1, and it all worked fine:

./configure --prefix=/tmp/GAP-PREFIX-4.12/usr --with-gmp=/opt/homebrew --with-readline=/opt/homebrew/opt/readline
make -j8
make install
cd /tmp/GAP-PREFIX-4.12/usr/lib/gap
mkdir pkg
cd pkg
tar xvf PATH/TO/PACKAGES.tar.gz

# launch GAP to verify it works, but doesn't autoload io (not yet compiled)
/tmp/GAP-PREFIX-4.12/usr/bin/gap

cd pkg/io
./configure   # works due to the location, otherwise should give $prefix/lib/gap as argument

# finally launch GAP and verify it loads IO now
/tmp/GAP-PREFIX-4.12/usr/bin/gap

@fingolfin
Copy link
Member Author

Maybe try inserting rm -rf pkg/io/bin into https://git.sagemath.org/sage.git/commit/?id=71c9fe337455d52637e0272fccdd633d8e96fa2e ?

@dimpase
Copy link
Member

dimpase commented Dec 1, 2022

So, how exactly do you build packages?

this is how we'll build gap 4.12.1:
https://github.com/sagemath/sagetrac-mirror/blob/u/dimpase/packages/gap/4_12_1/build/pkgs/gap/spkg-install.in

and here is how we build more gap packages:
https://github.com/sagemath/sagetrac-mirror/blob/u/dimpase/packages/gap/4_12_1/build/pkgs/gap_packages/spkg-install.in

anyway, I've backported the patch in the PR to 4.12.1, and everything is back to normal on macOS, too

@dimpase
Copy link
Member

dimpase commented Dec 1, 2022

To be sure, I just did a test with a fresh GAP 4.12.1, and it all worked fine

not sure what the test you mention is meant to test. to test this pr, one needs to fix #5234 to that io is ready

  • sorry, the shift keys on this laptop are not quite working...

@dimpase
Copy link
Member

dimpase commented Dec 1, 2022

Maybe try inserting rm -rf pkg/io/bin into https://git.sagemath.org/sage.git/commit/?id=71c9fe337455d52637e0272fccdd633d8e96fa2e ?

well, it's all fine as is, no extra copies of io.so anywhere...

@fingolfin
Copy link
Member Author

To be sure, I just did a test with a fresh GAP 4.12.1, and it all worked fine

not sure what the test you mention is meant to test. to test this pr, one needs to fix #5234 to that io is ready

The test refutes your claim in https://trac.sagemath.org/ticket/34391 elsewhere that there is a problem with io when built on macOS against an installed GAP: the test I quoted does exactly that, and works without experiencing the linker issues you had.

The patch in this PR fixes a genuine issue, but I believe that issue is merley a symptom of the problem in the Sage trac, not the root cause. Rather, I suspect the problem is with how Sage builds, tests and installs GAP.

well, it's all fine as is, no extra copies of io.so anywhere...

This misses the point: there is indeed no extra io.so. However, you are building io.so against the not yet installed GAP there, and then later "install" io by copying all files in that directory. I strongly suspect that the later attempt to build io again doesn't do that, but instead terminates because io.so already exists, and you end up installing the previously built io.so. Which exhibits the problem.

That's why I suggested to add rm -rf pkg/io/bin to build/pkgs/gap/spkg-check.in (alternatively, run make clean in it)

Of course this is just a guess, and it might be wrong, but I still think it would be prudent to do this cleaning in spkg-check.in.

@dimpase
Copy link
Member

dimpase commented Dec 1, 2022

To be sure, I just did a test with a fresh GAP 4.12.1, and it all worked fine

not sure what the test you mention is meant to test. to test this pr, one needs to fix #5234 to that io is ready

The test refutes your claim in https://trac.sagemath.org/ticket/34391 elsewhere that there is a problem with io when built on macOS against an installed GAP: the test I quoted does exactly that, and works without experiencing the linker issues you had.

there is no problem whatsoever with io loaded in a "classic" GAP session.

The patch in this PR fixes a genuine issue, but I believe that issue is merley a symptom of the problem in the Sage trac, not the root cause. Rather, I suspect the problem is with how Sage builds, tests and installs GAP.

well, it's all fine as is, no extra copies of io.so anywhere...

This misses the point: there is indeed no extra io.so. However, you are building io.so against the not yet installed GAP there,

why "not yet installed"?
GAP is installed (make install is run, in particular gap and gac executables are installed), while Sage package gap is installed. At this moment io is not installed.

Later, optionally, io is built and installed while Sage optional package gap_packages is installed.
At this moment gap, gac, base packages are already installed. And this, without the patch in this PR, results in
a broken io kernel module on macOS.

I have no idea why in this case the same io kernel module works with "classical" GAP, but does not work with libgap
(but this can be seen totally out of Sage, just with #5234.)

and then later "install" io by copying all files in that directory. I strongly suspect that the later attempt to build io again doesn't do that, but instead terminates because io.so already exists,

no, it does not exist at this point. Because io was not installed.

@fingolfin
Copy link
Member Author

OK, I don't know how Sage builds stuff, and playing guessing games with you when you refuse to try what I suggest is not leading anywhere. But that's fine by me, ultimately I don't care if there is a deeper issue with the Sage setup or not shrug

But you are wrong about #5234 resp. #5232 -- what you do there is build io.so against a non-installed gap (which does not use libgap), then try to load it with a different executable built using libgap. Yes, that does not work, and it isn't really supposed to work, but I made this PR here anyway as it is convenient to have it work.

But it is not needed if one properly builds io.so against an installed GAP.

@dimpase
Copy link
Member

dimpase commented Dec 1, 2022

OK, I don't know how Sage builds stuff, and playing guessing games with you when you refuse to try what I suggest is not leading anywhere.

I have no idea what you want me to try. I have explained to you that there are no stale copy of io.so around that might cause the issue.

But that's fine by me, ultimately I don't care if there is a deeper issue with the Sage setup or not shrug

But you are wrong about #5234 resp. #5232 -- what you do there is build io.so against a non-installed gap (which does not use libgap)

but "installed" GAP does not "use" libgap in any way. I just don't understand what you are talking about.

, then try to load it with a different executable built using libgap. Yes, that does not work, and it isn't really supposed to work, but I made this PR here anyway as it is convenient to have it work.

But it is not needed if one properly builds io.so against an installed GAP.

I don't understand. In Sage we are building io.so against installed, by make install, GAP.

@dimpase
Copy link
Member

dimpase commented Dec 1, 2022

In #5235 (comment) you attempted to "disprove" that there is nothing to fix if io.so was built properly. But it did not show that loading io.so from libgap works this way. There is no libgap there!

loading io from "classical" GAP kept working, so you're not showing anything new there at all.

@dimpase
Copy link
Member

dimpase commented Dec 1, 2022

In view of #5232 (comment), can we have a backport to 4.12 of this ?

@dimpase
Copy link
Member

dimpase commented Dec 1, 2022

(To be clear: in general kernel extensions are highly specific to the GAP they were built again, using one built against a non-installed GAP in a different executable built against libgap is not a good idea in general; but since it was trivial to get it working in this case, I did it; but on the long run I expect this to be problematic for other reasons, so I'd advise against it)

If this is your advice, you'd need to provide a way to build kernel extensions specifically for libgap - otherwise libgap won't be able to use them.
(Or perhaps it's easier to keep these built with gap compatible with libgap...)

@fingolfin
Copy link
Member Author

Sorry for the confusion: I missed that PR #5193 is not yet in stable-4.12 and hence also not in 4.12.1. So what I said above is only for GAP master and for the future 4.12.2.

@ChrisJefferson ChrisJefferson merged commit 6d95237 into gap-system:master Dec 7, 2022
@fingolfin fingolfin deleted the mh/fix-kext-ldflags branch December 7, 2022 13:08
@fingolfin
Copy link
Member Author

Backported to stable-4.12 via commit aed43ee

@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.12-DONE os: macos Issues and PRs that are (at least partially) specific to macOS release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot load kernel extensions built against non-install GAP into an executable built against libgap
3 participants