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

Replace build system with meson #24

Merged
merged 1 commit into from Sep 26, 2019

Conversation

MathieuDuponchelle
Copy link
Contributor

Meson is a fast, user-friendly build system, and many projects have already adopted it as a replacement for autotools.

This port has only been tested on Linux for now, I believe it should build fine on Windows / Mac but haven't tested these platforms so take it with a grain of salt.

I hope this is a welcome proposal :)

@MathieuDuponchelle
Copy link
Contributor Author

If that is of interest, meson is fairly easy to hook up with CI tools such as travis: https://mesonbuild.com/Continuous-Integration.html

@jbkempf
Copy link
Contributor

jbkempf commented Aug 28, 2019

I disagree on having an alternative build system: you should replace it completely.

Please follow the conventions we use on dav1d, for the meson file, though. Including proper licensing, warning levels, windows_flags and version_api +abi numbering

@MathieuDuponchelle
Copy link
Contributor Author

I disagree on having an alternative build system: you should replace it completely.

Fair enough, however I wouldn't feel comfortable proposing a patch doing that without testing on all the target platforms, and that's a larger time commitment than I'm ready to make right now :)

Please follow the conventions we use on dav1d, for the meson file, though. Including proper licensing, warning levels, windows_flags and version_api +abi numbering

I'll take a look, though it would be more helpful if you could make specific comments inline instead of asking me to infer conventions from a seemingly-unrelated project's build system ;)

@tguillem
Copy link
Contributor

Fair enough, however I wouldn't feel comfortable proposing a patch doing that without testing on all the target platforms, and that's a larger time commitment than I'm ready to make right now :)

You can still propose a patch and we will test it on all platforms.

@jbkempf
Copy link
Contributor

jbkempf commented Sep 5, 2019

I agree: we will test it.

@MathieuDuponchelle
Copy link
Contributor Author

There you go, removed autotools, took as much inspiration as I could from the dav1d build system, but some things you asked were not applicable (eg the previous build system didn't use a soname). Hope that's useful.

@MathieuDuponchelle MathieuDuponchelle changed the title Add meson as an alternative build system Replace build system with meson Sep 6, 2019
Copy link

@ePirat ePirat left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work on this! I did a quick review and noted some things inline.

meson.build Outdated Show resolved Hide resolved
meson.build Show resolved Hide resolved
meson.build Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
@jbkempf
Copy link
Contributor

jbkempf commented Sep 8, 2019

the previous build system didn't use a soname

Of course it did, but it was set to 0, which is OK for a new library.

Hope that's useful

It is, Thanks very much.

@jbkempf
Copy link
Contributor

jbkempf commented Sep 9, 2019

Anyway, this looks very nice, and in the right direction.

@MathieuDuponchelle
Copy link
Contributor Author

MathieuDuponchelle commented Sep 9, 2019

Of course it did, but it was set to 0, which is OK for a new library.

hrm sorry got confused with so version for a second here (that was in answer to your ABI numbering request) :)

src/microdns.pc.in Show resolved Hide resolved
meson.build Show resolved Hide resolved
@ePirat
Copy link

ePirat commented Sep 9, 2019

It seems there is still no (so)version set for the library.

cc @jbkempf

Unfortunately there is a little gotcha, libtool versioning works different from what one would expect so with version 0:0:0 on macOS you end up with:

libfoo.0.dylib (compatibility version 1.0.0, current version 1.0.0)

This is extremely confusing as the version in the file name does not match the compatibility version or current version…

With meson setting a version of 0.0.0 like so:

library('foo',
    files('libfoo.c'),
    install: true,
    build_by_default: true,
    version: '0.0.0'
)

results in

libfoo.0.dylib (compatibility version 0.0.0, current version 0.0.0)

as one might expect.

Of course this means that when we now set version 0.0.0 in meson, the filename will match with autotools-built versions but the actual version information will be different…
If this problem is limited to macOS though, we can ignore it, as there are probably not many users of microdns on macOS.

@jbkempf
Copy link
Contributor

jbkempf commented Sep 9, 2019

I don't think we care, for now, tbh.

@MathieuDuponchelle
Copy link
Contributor Author

Is there anything more I can do to help this go ahead, or is it simply a matter of someone finding time to test on the relevant platforms? I'll be testing this on a Windows (10) later, what other platforms will require testing?

@MathieuDuponchelle
Copy link
Contributor Author

Had to patch a few things here and there, but my branch now builds with MSVC, and both executables work just fine :)

Side note: I needed ping mdnshost.local to work as well, and the most straightforward I could find was to install Bonjour Print Services from https://support.apple.com/kb/dl999?locale=en_US , do you guys have a less weird recommendation to make?

@ePirat
Copy link

ePirat commented Sep 14, 2019

Can we maybe split MSVC support into a new PR?

@ePirat
Copy link

ePirat commented Sep 14, 2019

I don't think we should mix these two things into one discussion…

@MathieuDuponchelle
Copy link
Contributor Author

MathieuDuponchelle commented Sep 14, 2019

@ePirat no problem, though I'll propose things sequentially then. Removed my MSVC changes from the branch.

@MathieuDuponchelle
Copy link
Contributor Author

I've also tested with msys2 mingw64, the current state of the branch builds just fine there

@jbkempf
Copy link
Contributor

jbkempf commented Sep 14, 2019

LGTM. To me, this is good enough to merge, for now.

@MathieuDuponchelle
Copy link
Contributor Author

So, someone care to press the green button ? :)

@MathieuDuponchelle
Copy link
Contributor Author

Had forgotten to set install: true on the targets we want installed

@MathieuDuponchelle
Copy link
Contributor Author

ping :)

@ePirat
Copy link

ePirat commented Sep 20, 2019

Why did you add an uninstalled pc file? Did we have one before? It doesn't seem so.

@ePirat
Copy link

ePirat commented Sep 20, 2019

Additionally meson has no support for uninstalled pc files yet (afaik) using the proper module so I would prefer to not add one before meson supports that.

And personally I do not see the need for the uninstalled pc file variant.

@MathieuDuponchelle
Copy link
Contributor Author

Why did you add an uninstalled pc file? Did we have one before? It doesn't seem so.

There was no uninstalled PC file before no, but it's a nice practice. I don't depend on it anyway

@MathieuDuponchelle
Copy link
Contributor Author

@ePirat , removed, don't think this is worth an argument :) There's one other sore point however which I'd like to solve to be able to use libmicrodns as a subproject: renaming the src directory to microdns. That way the examples and external users will include microdns.h the same way, with a microdns/ prefix. Do you reckon this can make it into the MR? This point is very much related to the meson port IMHO

@MathieuDuponchelle
Copy link
Contributor Author

ping :)

@chouquette chouquette merged commit 2a2e3cd into videolabs:master Sep 26, 2019
@ePirat
Copy link

ePirat commented Sep 26, 2019

🎉

@MathieuDuponchelle
Copy link
Contributor Author

whee!

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

5 participants