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

Port build system to Meson #8

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

emilazy
Copy link
Contributor

@emilazy emilazy commented Aug 3, 2024

I was trying to use Debian’s CMake build system patch but ran into issues because its .pc file doesn’t actually include the linker flags required to link the resulting library.

I messed around with it for a bit, but generating correct .pc files from CMake is actually way too hard. So instead I just rewrote the build system in Meson, which is much simpler and takes care of all the details for us.

I don’t really expect this to be merged given the project has been inactive for over a decade, but I wanted to put it up here if any downstreams are interested in it.

@jonassmedegaard This may interest you, either to adopt directly in Debian or at least as a notification that the resulting .pc files from your current patch are broken; it lacks the flags for linking, and passes ones that don’t make sense like -ffile-prefix-map=/build/libresample-Yb23pu/libresample-0.1.3=.. (It also seems like the libresample.so in the current package is a broken symbolic link to libresample.so.1, but maybe that’s just some Debian thing I don’t fully understand.)

@minorninth
Copy link
Owner

Hi! Happy to take a contribution of Meson build files, but I'd prefer not to delete the autoconf/Make files - while they are pretty old-school, they still work fine - I just tried and confirmed that on a modern system they still work for me.

Modify it to just add the new build files, update the README to discuss both options, and I'm happy to merge!

@emilazy
Copy link
Contributor Author

emilazy commented Aug 3, 2024

I wasn’t expecting a response to this PR, let alone such a quick one :) Thanks for being receptive to the idea!

The reason I did this, rather than just going back to trying the autotools system when I ran into problems with the Debian CMake patch, is that we (Nixpkgs, but also other Linux distros – Fedora also ships a modified version of the CMake patch, for instance) want pkg-config .pc files for libraries that downstream projects can use. They can be mildly tricky to write correctly by hand in a way that works across all platforms, and Meson handles all the heavy lifting of generating correct ones. It’s really helpful if libraries officially produce them so that consumers of the code can rely on them rather than having to have complex fallback paths for cross‐platform library and compiler flag discovery.

How do you want me to handle this? Adapting the autotools build system to produce a correct .pc file is probably more work than I’m up for, but I’d be happy to mention that significant difference in the README.md and perhaps strongly recommend that downstream packagers use the Meson build system to ensure interoperability?

P.S. Some clarity on #6 would be helpful for us downstreams too (although I think “Two licenses available” in the README.md makes it reasonably clear that it’s SPDX OR like you’d expect).

@minorninth
Copy link
Owner

I tried to address #6 - hope that helps!

I'm fine with having Meson generate the .pc file! I don't want to ask you to adapt the autotools stuff, I just don't want to delete it since it works fine for some people. I know this project is embedded in many others and some of them may be using or even patching autotools, so getting rid of that would break them.

What would happen if you added the meson files and left the autotools there too? Just let people choose. Or would that break anything, like does it prevent Meson from working if those files are there?

@emilazy
Copy link
Contributor Author

emilazy commented Aug 3, 2024

Nope, it won’t break anything at all – just that of course only users of the Meson build system will get .pc files installed. If you think it’s reasonable to document that downstream distribution packagers ought to the Meson build system for this reason (so that people can hopefully generally rely on distribution packages of libresample containing pkg-config files, as is currently de facto the case due to widespread build system patching) I’m totally happy to redo this to leave the original build system in place.

@minorninth
Copy link
Owner

Sounds good!

@jonassmedegaard
Copy link

Thanks a lot, @emilazy - indeed th .pc file was broken in Debian, and works fine now with your patch (adapted to only add, not needlessly replate files).

@emilazy emilazy force-pushed the push-rrylzstqpytl branch 2 times, most recently from 1685f86 to da5cfaa Compare August 20, 2024 00:28
@emilazy
Copy link
Contributor Author

emilazy commented Aug 20, 2024

Sorry for the delay! I’ve now done as requested and left the Autotools build system alone, and documented the options. I’ve also somewhat presumptively bumped the version to 0.1.5 in the hopes that we can get this into a tagged release :)

I’ve left the Visual Studio 2005 file around as well, although Meson can generate project files for Visual Studio 2010 and newer, so that one might be less useful than the Autotools build and I can remove it if you agree. I’ve done testing to ensure that everything works properly with Meson on Windows, with the both the Ninja backend and the Visual Studio one, including compiling a DLL. I’ve confirmed it works natively on Windows with a Visual Studio toolchain, and with Nixpkgs’ MinGW-w64‐based cross‐compilation toolchain. It should work with a native MinGW toolchain too, but I didn’t test that. I opted for a traditional .def file over adding conditional __declspec(dllexport) constructions to the actual C code, though the latter would work too if you’d prefer.

I realized that the testresample and compareresample binaries don’t actually yield machine‐readable success/failure exit codes, so having them as Meson tests wasn’t quite appropriate. #7 fixes this for testresample as well as making the test actually pass, so I’ve left that in as a Meson test and highly recommend merging that PR before this one in order to have the test actually do something.

compareresample seems like it’s just there for manual comparison, so I left it as an optional feature that gets installed if enabled. Meson will currently enable it by default if libsndfile is present; not sure what your preference here is. I could make it not get installed (though that means it’s hard to run on Windows for DLL path reasons), or make it disabled by default. The status quo as of this PR seems fine to me since distro packages will be making explicit decisions about this anyway and are unlikely to include dependencies they don’t actively want in the build environment, but I’d lean towards keeping the automatic library detection and disabling the installation if you’re not fond of it.

Copy link
Owner

@minorninth minorninth left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Looks great.

@minorninth minorninth merged commit 7cb7f9c into minorninth:master Aug 23, 2024
@emilazy
Copy link
Contributor Author

emilazy commented Aug 23, 2024

Thanks for the quick merge and being open to this! I thought this would probably just sit around for other distros, so I really appreciate you taking the time to review :)

I’d definitely recommend merging #7 too, as the Meson test phase without that PR doesn’t meaningfully test anything and the current testresampleis broken in a way that that PR fixes.

Would we be able to get an official 0.1.5 tag on GitHub after that to package as a final release? We’re currently vendoring both this and #7 so that we can check the build works in CI.

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.

3 participants