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

BLD: fix type safety of version numbers (fix building on some systems) #101

Conversation

neutrinoceros
Copy link

@neutrinoceros neutrinoceros commented Mar 10, 2024

This fixes compilation of pyerfa on my system (MacOS AMD64) as reported in liberfa/pyerfa#141
However it's not clear to me what changed recently that made this patch (apparently) necessary. It's still possible that the change was specific to my setup, but I figure this patch is probably acceptable regardless.

@mhvk
Copy link
Contributor

mhvk commented Mar 10, 2024

This fails all tests, so is not the solution...

And it's weird - why is PACKAGE_VERSION_MAJOR seen as a string in the first place? Shouldn't it just be replaced by the preprocessor with a number?

We actually have two build systems, regular make and meson. Grepping, I see that in the former some care is taken to ensure there are no quotes:

erfa/m4/erfa-numver.m4

Lines 22 to 24 in df9f5c4

AC_DEFINE_UNQUOTED(PACKAGE_VERSION_MAJOR, $MAJOR, [Define to the major version of this package.])
AC_DEFINE_UNQUOTED(PACKAGE_VERSION_MINOR, $MINOR, [Define to the minor version of this package.])
AC_DEFINE_UNQUOTED(PACKAGE_VERSION_MICRO, $MICRO, [Define to the micro version of this package.])

While I'm less sure about meson.built:

erfa/meson.build

Lines 22 to 29 in df9f5c4

# API versions
version = meson.project_version().split('.')
confdata.set_quoted('PACKAGE_VERSION', meson.project_version())
confdata.set('PACKAGE_VERSION_MAJOR', version[0])
confdata.set('PACKAGE_VERSION_MINOR', version[1])
confdata.set('PACKAGE_VERSION_MICRO', version[2])
confdata.set_quoted('SOFA_VERSION', '20231011')

Though the fact that sofa uses set_quoted suggests to me that the others should normally be OK...

@neutrinoceros
Copy link
Author

I see. Let me try and figure out what compiler/build system pip is even using under the hood.

@neutrinoceros neutrinoceros force-pushed the bld/explicit_char2int_conversion branch from 5568513 to 1701553 Compare March 10, 2024 14:44
@neutrinoceros neutrinoceros changed the title BLD: fix building against compiler that refuses to convert char to int implicitly BLD: fix type safety of version numbers (fix building on some systems) Mar 10, 2024
@neutrinoceros
Copy link
Author

neutrinoceros commented Mar 10, 2024

I replaced the patch with a completely different approach. Hopefully this is the actual fix this time ("it works on my machine" ™️).

@mhvk
Copy link
Contributor

mhvk commented Mar 10, 2024

It would also be good to have a separate run with a setup more like yours (we already have MacOS, though)...

@mhvk
Copy link
Contributor

mhvk commented Mar 10, 2024

Sadly, the new approach doesn't work either... I think my link to meson.built may have been a red herring.

@neutrinoceros
Copy link
Author

I guess I messed up my local testing too, this doesn't look right. Anyway, I'll get back to this later.

@neutrinoceros
Copy link
Author

I actually can't seem to reproduce the problem today, even starting fresh with a clean copy of the repo. Pretty odd, but no point in keeping this PR alive then. Thanks for your feedback and sorry for the noise !

@neutrinoceros neutrinoceros deleted the bld/explicit_char2int_conversion branch March 11, 2024 09:33
@mhvk
Copy link
Contributor

mhvk commented Mar 11, 2024

Very weird indeed, but no worries about the noise!

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.

2 participants