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

meson: create full dist tarball with autoconf artifacts included #100

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

eli-schwartz
Copy link
Contributor

No description provided.

@mhvk
Copy link
Contributor

mhvk commented Dec 17, 2023

Failures on both MacOS and Windows... For those, autotools doesn't work? Also, in #97 (comment), the comment was partially about configure being absent. Should that be included too? Let me ping @sergiopasra and @astrofrog ...

@eli-schwartz
Copy link
Contributor Author

Failures on both MacOS and Windows

Right, good point... we added windows CI as part of adding meson support!

For macOS, the CI runs brew install autoconf automake libtool but only when testing the autotools build. So we should go add it there too, in order to run a successful distcheck.

Also, in #97 (comment), the comment was partially about configure being absent. Should that be included too?

If you run the autogen logic (really just autoreconf) in the dist root for meson, then the files it creates get included in meson's meson dist produced tarball.

@mhvk
Copy link
Contributor

mhvk commented Dec 18, 2023

Looks good to me, but probably best if @sergiopasra has a look too, to see whether this now is all OK.

meson.build Outdated
@@ -39,3 +39,7 @@ import('pkgconfig').generate(
name: 'Erfa',
description: 'Essential Routines for Fundamental Astronomy',
)

if build_machine.system() != 'windows'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't have the time to look all details, but my question is: is the dist package supposed to be platform independent? I assume that we are talking about a source distribution.
In this case the file should be included in any case, including on windows platforms, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory yes, in practice autotools wasn't actually supported on Windows and can't generate a configure script there.

So the choices are probably:

  • run build and test on Windows CI but not dist
  • create different dists on Windows vs on other platforms.

I'm okay with either approach, so I can switch to the former.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory yes, in practice autotools wasn't actually supported on Windows and can't generate a configure script there.

OK understood.

I think that as soon as we distribute the complete source package (the one generated on unix) it is fine to keep the dist workflow also on windows.

... please feel free to proceed as you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's drop the distcheck on Windows.

Requires autotools installed to run `meson dist`. This won't work on
Windows, which erfa only supports via meson. A full distcheck isn't
needed there.
# if running from `meson dist`, make sure we run from the dist root
if [ -n "$MESON_PROJECT_DIST_ROOT" ]; then
cd "$MESON_PROJECT_DIST_ROOT"
# meson < 0.58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just bump our meson version requirement (I seem to be on 1.2.3 myself).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an option, but it does mean Debian Oldstable and Ubuntu 20.04 LTS users, for example, would not be able to use the version of meson installed via apt and would have to manually acquire an updated meson.

I don't like to assume what range of userbase a project wishes to support. I'll go with whatever makes you happy. (And it was easy for me to do, anyway.) :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, missed that last comment... no point changing it now any more! We'll just get it whenever we next look at this.

@mhvk
Copy link
Contributor

mhvk commented Dec 29, 2023

OK, let's get this in. Thanks @eli-schwartz!

@mhvk mhvk merged commit df9f5c4 into liberfa:master Dec 29, 2023
5 checks passed
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.

4 participants