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 Build #263

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

Kangie
Copy link
Contributor

@Kangie Kangie commented Oct 1, 2024

Hi Team,

While logging #262 I had the thought that it ought to be relatively straightforward to port Dillo to Meson.

I took a bit of time this afternoon and the result is an initial port.

It builds and links without complaint, but it's currently not producing a "working" dillo - I've missed something with DNS that I'll come back to shortly.

I know this wasn't specifically requested however I still think it's worthwhile:

  • It's possible to run a 'meson' build using muon and samurai which are C99 implementations of Meson and Ninja respectively.
  • It's a lot smaller than automake + autoconf (coming in at just over half the lines of "code" to maintain, which could be reduced by adjusting the blind formatting rules I used)
  • It's faster / more efficient
    • sh -c 'meson setup --wipe build ; ninja -C build' 18.87s user 7.74s system 958% cpu 2.777 total
    • sh -c './configure && make -j' 33.55s user 4.55s system 550% cpu 6.917 total
  • It doesn't rely on which ;)

image

image

I'll keep plugging away at this unless you're not interested until we can get it ready for release.

Closes: #262

Cheers,

Matt

@rodarima
Copy link
Member

rodarima commented Oct 1, 2024

Thanks for the PR and sorry for the late reply.

Even if I hate autotools with all my heart, they are capable of producing a unreadable but dependency-free configure script that can run in (almost?) any POSIX-compliant machine, including very old ones.

There are other issues with autotools too, which are mostly the reason I'll probably change the build system in the future, but I don't like the idea of adding python as a build dependency because of meson (or using the WIP muon).

As FLTK has switched recently to cmake, and it is very likely that it has to be supported in every platform that we want to build Dillo on, we can asume that cmake is already available due to indirect build dependencies. So I will be inclined to choose it over meson, which should yield similar speeds when using ninja instead of make.

In any case, I won't take this decision without making a study beforehand of the options, and see which one is more appropriate. Also consider the fact that I have already some knowledge on cmake, and almost none on meson. Regardless, this change will cause a major version bump, so it will not likely happen soon.

Regarding the which problem, it should be a ~5 line patch, so I don't think it is a reason to change the build system (yet). The IWYU patch is also welcome.

@rodarima rodarima marked this pull request as draft October 1, 2024 17:03
@Kangie
Copy link
Contributor Author

Kangie commented Oct 1, 2024

or using the WIP muon.

Having recently done the fvwm3 meson port where Muon was a desired feature, I can say that the recent 0.3.0 release is effectively on-par with mainline Meson (so no hard Python req)

As FLTK has switched recently to cmake, and it is very likely that it has to be supported in every platform that we want to build Dillo on, we can asume that cmake is already available due to indirect build dependencies. So I will be inclined to choose it over meson, which should yield similar speeds when using ninja instead of make.

CMake is "fine" however IMO Meson results in far more maintainable and readable build system code, and typically one obvious way to accomplish any given task rather than the ~5 ways that there are to do it in CMake (with no indication which is more appropriate for a given situation).

Which platforms that we're building C++11 code on don't support C99 and/or Python anyway ;)

With 99% of the porting done you shouldn't need to dig into the guts for quite a while.

I may submit a patch for the which issue at some point; got to do actual work today. I will, however, get the Meson build to the point that the browser is actually functional at some point so that you can compare.

@rodarima
Copy link
Member

rodarima commented Oct 2, 2024

With 99% of the porting done you shouldn't need to dig into the guts for quite a while.

I need to understand very deeply the build system as to debug weird problems in all kinds of platforms I don't have access to, other than a mail/IRC channel with the person. So, totally the opposite.

I may submit a patch for the which issue at some point; got to do actual work today.

Thanks, that would be nice.

@Kangie
Copy link
Contributor Author

Kangie commented Oct 15, 2024

Following up on this, it looks like it's (almost) all working:

image

build/src/dillo
paths: Cannot open file '/home/kangie/.dillo/dillorc': No such file or directory
paths: Cannot open file '/usr/local/etc/dillodillorc': No such file or directory
paths: Using internal defaults...
paths: Cannot open file '/home/kangie/.dillo/keysrc': No such file or directory
paths: Cannot open file '/usr/local/etc/dillokeysrc': No such file or directory
paths: Using internal defaults...
paths: Cannot open file '/home/kangie/.dillo/domainrc': No such file or directory
paths: Cannot open file '/usr/local/etc/dillodomainrc': No such file or directory
paths: Using internal defaults...
dillo_dns_init: Here we go! (threaded)
TLS library: OpenSSL 3.3.2 3 Sep 2024
Disabling cookies.
paths: Cannot open file '/home/kangie/.dillo/hsts_preload': No such file or directory
paths: Cannot open file '/usr/local/etc/dillohsts_preload': No such file or directory
paths: Using internal defaults...
Nav_open_url: new url='about:splash'
Nav_open_url: new url='https://www.google.com'
Dns_server [0]: www.google.com is 142.250.67.4 2404:6800:4006:811::2004
Connecting to 142.250.67.4:443
www.google.com: TLSv1.3, cipher TLS_AES_256_GCM_SHA384
sha256 256-bit EC: /CN=www.google.com
sha256 2048-bit RSA: /C=US/O=Google Trust Services/CN=WR2
sha256 4096-bit RSA: /C=US/O=Google Trust Services LLC/CN=GTS Root R1
root: /C=BE/O=GlobalSign nv-sa/OU=Root CA/CN=GlobalSign Root CA
>>>> a_Nav_repush <<<<
Nav_open_url: new url='https://www.google.com'
a_Nav_expect_done: repush!

Not sure why the PNG isn't visible but that should be straightforward to fix, and the etcdir just needs a trailing slash.

@Kangie
Copy link
Contributor Author

Kangie commented Oct 15, 2024

OK, making progress... I had HAVE_PNG not ENABLE_PNG.

This is looking good:

image

aths: Cannot open file '/home/kangie/.dillo/dillorc': No such file or directory
paths: Cannot open file '/usr/local/etc/dillo/dillorc': No such file or directory
paths: Using internal defaults...
paths: Cannot open file '/home/kangie/.dillo/keysrc': No such file or directory
paths: Cannot open file '/usr/local/etc/dillo/keysrc': No such file or directory
paths: Using internal defaults...
paths: Cannot open file '/home/kangie/.dillo/domainrc': No such file or directory
paths: Cannot open file '/usr/local/etc/dillo/domainrc': No such file or directory
paths: Using internal defaults...
dillo_dns_init: Here we go! (threaded)
TLS library: OpenSSL 3.3.2 3 Sep 2024
Disabling cookies.
paths: Cannot open file '/home/kangie/.dillo/hsts_preload': No such file or directory
paths: Cannot open file '/usr/local/etc/dillo/hsts_preload': No such file or directory
paths: Using internal defaults...
Nav_open_url: new url='about:splash'
Nav_open_url: new url='http://www.google.com'
Dns_server [0]: www.google.com is 142.251.221.68 2404:6800:4006:809::2004
Connecting to 142.251.221.68:80
>>>> a_Nav_repush <<<<
Nav_open_url: new url='http://www.google.com'
a_Nav_expect_done: repush!

@Kangie
Copy link
Contributor Author

Kangie commented Oct 15, 2024

Sorry about the spam, but best to keep this commentary separate:

Regardless, this change will cause a major version bump, so it will not likely happen soon.

There's little reason (aside from keeping autotools in the repo...) not to include the meson build files alongside autotools for a transitional period, then you are able to deprecate autotools and remove it at a time of your choosing with the confidence that it's not going to suddenly cause downstream issues.

@Kangie Kangie force-pushed the a-proper-build-system branch 22 times, most recently from 62e57c9 to 219fd24 Compare October 15, 2024 04:03
@Kangie Kangie force-pushed the a-proper-build-system branch 9 times, most recently from eb60812 to 23a466d Compare October 15, 2024 22:51
@Kangie
Copy link
Contributor Author

Kangie commented Oct 15, 2024

@rodarima

Thoughts? I'm having a bit of trouble with the HTML tests in CI (frankly I can barely get them to run on Gentoo at the best of times) but otherwise we have a working browser and test suite.

Cygwin is unfortunate, but it was building and passing tests before I set the required meson version to just above what is currenly packaged. Theoretically pip will work, just pushed that change; can follow up and request a slightly newer meson be packaged.

@rodarima
Copy link
Member

I'm not sure what problem you are trying to solve here, but I see you are putting a lot of effort into something we have not requested or encouraged. There are plenty of issues here that I would like someone to help with: https://github.com/dillo-browser/dillo/issues

Autoconf/autotools is doing an acceptable job as build system. It is slow but that shouldn't be a big problem when Dillo is mainly targeted towards slow machines where the bottleneck is the build (not the scheduling of the build). Saving some seconds in the build process is not very relevant when we can produce a portable tarball release.

I have a limited time to spend on the project, so I will prioritize not changing the build system or choosing one that I'm familiar with, rather than learning a new one. I will not maintain two build systems, when we switch (if we do) we will remove autotools.

Keep in mind that we can only test a very limited amount of (modern) platforms right now, and I will not try to switch the build system until I have determined which platforms we decide to break and which ones to continue to support. Before that point, I would like to have improved the CI so we also test Dillo on older hardware. Until that happens, I will not change the build system.

I think this PR may be useful to compare with cmake or others in the future (I have saved the patch), but from the knowledge I have right now it is very unlikely that we end up switching to meson/muon.

Fixes for includes would be nice to merge on their own.

I know there are current problems with the current build system. For example, we don't support cross-compilation, mainly by the way we query FLTK and other flags, but that can also be fixed independetly without changing the build system.

I'm having a bit of trouble with the HTML tests in CI (frankly I can barely get them to run on Gentoo at the best of times)

The HTML test suite is not designed to run by non-developers, the same way I don't expect you to pass the WPT tests when packaging Firefox or Chromium. But essentially, we open the test page on dillo in a virtual X server and take a snapshot, then we do the same with a reference page and we compare the pixels one by one. The two images are saved, so you may be able to see what went wrong. Also, those tests require a vanilla dillorc and style.css, otherwise you will likely break them. If they continue to break, please address it in a new issue, as they shouldn't break.

@Kangie
Copy link
Contributor Author

Kangie commented Oct 16, 2024

but I see you are putting a lot of effort into something we have not requested or encouraged

Making the world a better place is its own reward.

Additionally, as a downstream packager/maintainer it ended up being more effective to rewrite the whole build system than deal with the mess that is the existing autotools impl. I also really don't want to deal with more downstream tickets or additional porting work due to the (at best, charitably described as) legacy way in which the current Autotools implementation does things.

There are plenty of issues here that I would like someone to help with: https://github.com/dillo-browser/dillo/issues

I would be more inclined to help if I didn't have to deal with a poor autotools impl, and I'm not inclined to fix autotools when the work on meson is already done.

so I will prioritize not changing the build system or choosing one that I'm familiar with, rather than learning a new one. I will not maintain two build systems, when we switch (if we do) we will remove autotools.

As the current maintainer of Dillo, that's your prerogative. I can point at other examples where projects aimed at low spec / older machines and which value portability have gone with precisely this approach, and picked Meson.

I will not try to switch the build system until I have determined which platforms we decide to break and which ones to continue to support

As a C++ project you can assume that C99 compilers will be available to provide muon and samurai to substitute for meson and ninja respectively on platforms that don't have (or want) a Python dependency.

If your platforms need C++11 and don't support C99 that is, honestly, their problem. Users can always continue to use an older tarball - who actually updates these legacy systems you're concerned about frequently, and isn't likely to just grab an older tarball?

I know there are current problems with the current build system. For example, we don't support cross-compilation, mainly by the way we query FLTK and other flags, but that can also be fixed independetly without changing the build system.

The TL;DR is that I've already done all of the work for you on each of the issues that you've just listed.

If you want to throw that away because you already have "some knowledge" of cmake that is, of course, your choice.

Having already admitted that you already have limited time to work on this project, and with volunteers submitting fixes for deficiencies in the existing build system that you are aware of, it really seems like you're shooting yourself in the foot by not considering this.

Saving some seconds in the build process is not very relevant when we can produce a portable tarball release.

Saving time in CI/downstream delivers on measurable benefits to both energy consumption and to the people that have to actually verify their own bulids.

Additionally the reduced complexity of Meson (and the use of a matrix to provide better CI/CD coverage in a concise manner) lends itself to a more maintainable build system going forward. How many autotools issues have not been fixed because it's terrible spaghetti that's too hard to parse?

Fixes for includes would be nice to merge on their own.

git cherry-pick ...

The HTML test suite is not designed to run by non-developers

Conveniently I, as a downstream packager, do want to know that my package behaves as expected before pushing it out to my users.

If they continue to break, please address it in a new issue, as they shouldn't break.

I'm really not inclined to touch anything to do with autotools in my limited time for this project.

@rodarima
Copy link
Member

The TL;DR is that I've already done all of the work for you on each of the issues that you've just listed.

This is far from the reality.

You have done a fair amount of work to help yourself avoid dealing with autotools (which is understandable) but you have not really considered the implications of this change for the project and other users.

The expensive part of the work is not porting this to a new build system, but maintaining it over the years and identifying issues reported by users from other platforms I don't have access to.

You have continued to work on this on your own, even if I have already explained to you that I will evaluate this in the future not now. I cannot stop you from spending your own time into whatever it is you what to spend it.

Your current solution continues to rely on the fltk-config binary, which won't work for cross compilation, which is one of the main issues I want to solve when switching the build system. I prioritized cmake because they can read the information from the .cmake module of FLTK directly:

https://cmake.org/cmake/help/latest/module/FindFLTK.html
https://github.com/Kitware/CMake/blob/fa61269d8e6e75448437cf9071cde97ecb35e054/Modules/FindFLTK.cmake#L163-L207

If you want to throw that away because you already have "some knowledge" of cmake that is, of course, your choice.

As the current maintainer of Dillo, that's your prerogative

git cherry-pick ...

The hostile behavior is not helping. I suggest you consider how you what to approach the FOSS comunity.

Conveniently I, as a downstream packager, do want to know that my package behaves as expected before pushing it out to my users.

And I, as the person who wrote the test infrastructure as well as those tests, I'm telling you that they are not designed for you to run as packager. They are designed to avoid introducing regressions in the layout engine. Same with RTFL, which I have no idea why you enabled it under the "debug" flag.

If you still insist into running them, I recommend you read the output logs to see what is wrong rather than blaming autotools:

https://933451.bugs.gentoo.org/attachment.cgi?id=894972

+ xwd -id 0x200009 -silent
+ convert xwd:- png:white-space.html_x8y/html.png
client(400000): Reserved pid(931).
client(400000): Reserved cmdname(xwd) and cmdargs(-id 0x200009 -silent).
AllocNewConnection: client index = 2, socket fd = 8, local = 1
convert: unable to open image 'xwd:-': No such file or directory @ error/blob.c/OpenBlob/3571.

Your convert(1) program doesn't understand xwd:- for some reason. We may be able to place it in a temporary directory instead of using a pipe, not sure what is causing this problem on your end (maybe a config switch on convert).

I really don't have any interest in pursuing this discussion futher, as I want to focus on the next 3.2.0 release. I understand your position, and I will come back to reevaluate this in the future, hopefully when we have some more exotic machines to test a build system on.

@Kangie
Copy link
Contributor Author

Kangie commented Oct 18, 2024

You have continued to work on this on your own, even if I have already explained to you that I will evaluate this in the future not now

You need a working (or mostly-working) PR to evaluate; the initial state was 'it builds'. The current PR is mostly feature-complete, however obviously the HTML tests aren't running properly in CI (likely down to how the script is invoked via meson), and some of the more debug-y options are not yet implemented (efence, gprof, insure) - That should be enough for you to decide whether or not you want to proceed further down this path.

edit: HTML tests actually appear to be working locally, but never returning properly - the test output appears to show expected tests and failures but Meson's test harness isn't happy and if the timeout is disabled the tests "run" indefinitely - I'll look into the test script at some point, maybe.

Edit 2: First HTML test terminated successfully without intervention in just over an hour so they will eventually complete.

Edit 3: After remembering to install Dillo into a prefix I'm able to successfully run the HTML rendering tests via meson (using the binary in builddir/src) locally. Oops. I suspect that the missing files may have been the cause of the hanging tests. There are a number that now actually fail based on the image comparison; I'll look into the html - I didn't have everything enabled in terms of image formats and that's a likely culprit.

Your current solution continues to rely on the fltk-config binary, which won't work for cross compilation, which is one of the main issues I want to solve when switching the build system. I prioritized cmake because they can read the information from the .cmake module of FLTK directly:

https://cmake.org/cmake/help/latest/module/FindFLTK.html https://github.com/Kitware/CMake/blob/fa61269d8e6e75448437cf9071cde97ecb35e054/Modules/FindFLTK.cmake#L163-L207

Thanks for the pointer, I'd assumed that was coming with upstream's port to CMake that you mentioned; I've pushed a commit that will use this to detect FLTK (which I'll rebase and squash at some point soon); turns out it needed FLTK not fltk. I suspect that it's going to fall back to fltk-config a lot of the time anyway; upstream probably still need to ship a pkgconfig file. This will likely be true regardless of build system.

If you want to throw that away because you already have "some knowledge" of cmake that is, of course, your choice.

As the current maintainer of Dillo, that's your prerogative

git cherry-pick ...

The hostile behavior is not helping. I suggest you consider how you what to approach the FOSS comunity.

I'm sorry that you feel that this is hostile, there has been some miscommunication; It's your project, it's your choice whether or not you accept offered help - I'm not going to be bitter if you go with CMake (though if you hit the '5 ways to do something with no obvious best option' I may drop-in for an 'I told you so').

On my part I'm a bit of a build system buff who has worked on / ported multiple projects, and I have seen many poor CMake implementations in-the-wild: I regularly work on Autotools, CMake, GN, Meson, and hand-rolled scientific computing Makefiles.

I'm also involved in upstream fixes to many packages that I maintain; I'd consider your examples to be 'direct' or 'blunt' communication but I'll take it under consideration.

I'm not likely to submit additional PRs for the trivial fixes, please feel free to cherry pick them; I'll sort out any conflicts when I rebase.

As a real carrot for a proper build system, tools like include-what-you-use will make identifying future instances of this trivial and comes "free" with CMake or Meson.

Same with RTFL, which I have no idea why you enabled it under the "debug" flag.

This:

dillo/configure.ac

Lines 74 to 75 in 572b934

AC_ARG_ENABLE([rtfl],
[AS_HELP_STRING([--enable-rtfl], [Print low-level RTFL messages for debugging the renderer (very large slowdown)])],

debug seems more appropriate than rtfl based on the comment, and enabling -Ddebug seems like something no reasonable person would use in anger in the real-world. When it comes time to evaluate, and if you decide to proceed, please ping me with any desired changes, this is a first-pass 'port autotools effectively as-is', not a final state.

Your convert(1) program doesn't understand xwd:- for some reason. We may be able to place it in a temporary directory instead of using a pipe, not sure what is causing this problem on your end (maybe a config switch on convert).

Appreciate the pointer; I've pushed a commit to include the missing X11 dependencies on ImageMagick.

I want to focus on the next 3.2.0 release. I understand your position, and I will come back to reevaluate this in the future, hopefully when we have some more exotic machines to test a build system on.

Evaluate at your leisure, please don't think I was implying that this should replace the build system for your upcoming release. As downstream packager I would love to see any replacement for Autotools considered at some point after that.

As a final note I would like you to consider running Autotools and Meson (or CMake) side-by-side for a transition period - we can add a -Dexperimental flag a la freeciv to indicate to users that it's not the 'default' build path (and make them explicitly opt-in) and we can roadmap the deprecation - this should enable users with those exotic systems to provide feedback well before the legacy build is in any danger of being removed. The updated matrix CI should make maintaining them side-by-side far easier :)

@Kangie Kangie force-pushed the a-proper-build-system branch 8 times, most recently from 618c06b to 94a8c81 Compare October 20, 2024 12:43
It's been a mandatory part of POSIX since 2001 an although
it's an ISO C Extension, cstdint is ISO for C++

Signed-off-by: Matt Jolly <[email protected]>
This is a first pass at porting autoconf/automake to Meson.

Signed-off-by: Matt Jolly <[email protected]>
Use Matrix configurations to make a more concise config that
builds both the Autotools and Meson build paths on supported
platforms, and where possible uses Clang and GCC

Signed-off-by: Matt Jolly <[email protected]>
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.

Configure uses which which is not POSIX compliant
2 participants