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

Parse time_t, off_t, etc. from GIR XML #430

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iliastsi
Copy link

@iliastsi iliastsi commented Mar 30, 2024

Up until now, g-ir-scanner would map these types to lower-level types at scan time by assuming that time_t was an alias for long, off_t was an alias for size_t and so on. This is not always accurate: some ILP32 architectures have 64-bit time_t (for Y2038 compatibility) and 64-bit off_t (for large file support) [1].

In order to resolve this incompatibility, giscanner now treat these as standalone types [2].

Patch nameToBasicType to parse these types that now appear in GIR XML.

Without this patch, gi-glib fails to build on Debian [3] which uses the newer gobject-introspection [4].

[1] https://gitlab.gnome.org/GNOME/gobject-introspection/-/issues/494
[2] https://gitlab.gnome.org/GNOME/gobject-introspection/-/merge_requests/455
[3] https://bugs.debian.org/1067272
[4] https://bugs.debian.org/1066032

Up until now, g-ir-scanner would map these types to lower-level types at
scan time by assuming that time_t was an alias for long, off_t was an
alias for size_t and so on. This is not always accurate: some ILP32
architectures have 64-bit time_t (for Y2038 compatibility) and 64-bit
off_t (for large file support) [1].

In order to resolve this incompatibility, giscanner now treat these
as standalone types [2].

Patch 'nameToBasicType' to parse these types that now appear in GIR XML.

[1] https://gitlab.gnome.org/GNOME/gobject-introspection/-/issues/494
[2] https://gitlab.gnome.org/GNOME/gobject-introspection/-/merge_requests/455
@garetxe
Copy link
Collaborator

garetxe commented Apr 2, 2024

Awesome, thanks!

Would it be possible to expose these types in the generated bindings using the types in System.Posix.Types, instead of just ints?

I think it might not be too hard: add the new types to BasicType, add the new cases to haskellBasicType (ideally with a prefix, so something like S.P.T.COff and so on), and then add the necessary import to the generated code, in Code.hs:

import qualified System.Posix.Types as S.P.T

Sorry for the extra work! I would happily do this myself, but I don't have the new version of the libraries in my system yet, so I cannot easily test, and I worry something might break.

@iliastsi
Copy link
Author

iliastsi commented Apr 4, 2024

Hi @garetxe,

I can definitely do that, but I am not sure why this is needed. Why we want to add these types to BasicType when for example gshort is implemented as an int?

nameToBasicType "gshort" = case sizeOf (0 :: CShort) of
2 -> Just TInt16
4 -> Just TInt32
8 -> Just TInt64
n -> error $ "Unexpected short size: " ++ show n
nameToBasicType "gushort" = case sizeOf (0 :: CUShort) of

Or are you suggesting we should add gshort, gsize etc to BasicType as well?

Also, I notice that the upstream girparser doesn't have these types on BaseicTypeInfo as well (see https://gitlab.gnome.org/GNOME/gobject-introspection/-/blob/42dc3ab42458d2582377d867a0f9e4cd390ac036/girepository/girparser.c#L489) and they convert them to ints instead (see https://gitlab.gnome.org/GNOME/gobject-introspection/-/blob/42dc3ab42458d2582377d867a0f9e4cd390ac036/girepository/girparser.c#L469).

I must say I have no prior experience with gobject-introspection, all I did was to copy what they did on the C code here (https://gitlab.gnome.org/GNOME/gobject-introspection/-/merge_requests/455). Any help to clarify things would be more than welcome.

@garetxe
Copy link
Collaborator

garetxe commented Apr 6, 2024

I can definitely do that, but I am not sure why this is needed. Why we want to add these types to BasicType when for example gshort is implemented as an int?

Yes, good point. In retrospect I think we should be representing gshort as CShort too. (I am not suggesting to do that in this PR, though!)

The main reason for doing this is portability of the generated Haskell code: as things stand right now if the underlying size of the type changes between architectures one would need to use something like CPP in the Haskell code using the bindings in order to support multiple architectures.

@iliastsi
Copy link
Author

Thanks for the explanation @garetxe, it makes sense. We can address this in a subsequent PR.

I see that build fails for older versions of GHC (older than 8.8) since CSocklen was introduced in GHC 8.10. How do you suggest we address this? Shall we just drop support for these GHCs that don't work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants