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

Make MesonNinja respect the toolchainopts with buildtype as well as --debug and --optimization flags #3454

Merged
merged 7 commits into from
Oct 11, 2024

Conversation

Micket
Copy link
Contributor

@Micket Micket commented Sep 20, 2024

Fixes #3280

Should also merge easybuilders/easybuild-easyconfigs#21619 once this is merged.

In this version, i double up on both buildtype as well as --optimizationand --debug flags to respect the toolchainopts. I think this will be the better approach, but i haven't tested anything yet.

@boegel boegel changed the title Make mesonninja respect the toolchainopts with buildtype as well as --debug and --optimization flags Make MesonNinja respect the toolchainopts with buildtype as well as --debug and --optimization flags Sep 23, 2024
boegel
boegel previously requested changes Sep 25, 2024
easybuild/easyblocks/generic/mesonninja.py Outdated Show resolved Hide resolved
@Micket
Copy link
Contributor Author

Micket commented Sep 27, 2024

WARNING: Recommend using either -Dbuildtype or -Doptimization + -Ddebug. Using both is redundant since they override each other. See: https://mesonbuild.com/Builtin-options.html#build-type-options

ok, so apparently meson doesn't want you to do this. I find this strange, as just using the optimization + debug options wouldn't cover things like -DNDEBUG defines. Maybe noone using meson uses such defines in their C/C++ codes? :/

@Micket
Copy link
Contributor Author

Micket commented Sep 27, 2024

mesonbuild/meson-python#325 No this seems to indicate otherwise. We certainly always want the NDEBUG set for our builds...

edit: see also https://mesonbuild.com/Release-notes-for-0-45-0.html#b_ndebug-ifrelease

@Micket
Copy link
Contributor Author

Micket commented Oct 1, 2024

Alright, so to summarize

  1. I add the -v flag so we can see what we are compiling
  2. I set the build type, in case they have some "if-release" logic that adds extra flags and such.
  3. I set optimization and debug on top of that, which produces a warning that I disagree with due to the point mentioned above.
  4. I set the -bn_debug=true flag for all builds that aren't noopt, so that we get -DNDEBUG when building debugoptimized as well as release. I think by default, the meson-python actually adds the flag -bn_debug=if-release but if we are enabling debug symbols, you don't want to squash this. They mention in the issue tracking n_debug flag that they don't add it automatically to release because they expect packagers to set all their flags themselves anyway.. and i guess they are right (now).

All in all this brings meson builds in line with the rest of the stuff we build, and I don't think any of it is controversial.
Wouldn't surprise me if we see a large speedup in some packages after these changes, but it's hard to test things like "cairo" or "Wayland" for performance.

@Micket
Copy link
Contributor Author

Micket commented Oct 1, 2024

Test report by @Micket

Overview of tested easyconfigs (in order)

  • SUCCESS cairo-1.18.0-GCCcore-13.3.0.eb
  • SUCCESS GLib-2.78.1-GCCcore-13.2.0.eb
  • SUCCESS libglvnd-1.7.0-GCCcore-13.3.0.eb
  • SUCCESS Wayland-1.23.0-GCCcore-13.3.0.eb

Build succeeded for 4 out of 4 (4 easyconfigs in total)
vera-skylake-build - Linux Rocky Linux 8.9, x86_64, Intel Xeon Processor (Skylake, IBRS, no TSX), Python 3.6.8
See https://gist.github.com/Micket/1c22578d1e11a2d81ddd7f38a3c0030e for a full test report.

@Micket
Copy link
Contributor Author

Micket commented Oct 1, 2024

Test report by @Micket

Overview of tested easyconfigs (in order)

Build succeeded for 3 out of 4 (4 easyconfigs in total)
vera-skylake-build - Linux Rocky Linux 8.9, x86_64, Intel Xeon Processor (Skylake, IBRS, no TSX), Python 3.6.8
See https://gist.github.com/Micket/d182683aa7be4718a6ad9167888a51d9 for a full test report.

@Micket Micket force-pushed the mesonrelease2 branch 2 times, most recently from e260fbf to 7f1e5fc Compare October 1, 2024 16:18
@Micket
Copy link
Contributor Author

Micket commented Oct 1, 2024

So, Wayland apparently needs to be built with asserts, else it fails the test suite. Which means, one can't actually test a true install... Not sure how much it impacts performance, or if we care about tools like wayland for that sort of performance in HPC settings, so I'm making n_debug into an option.

@Micket Micket force-pushed the mesonrelease2 branch 2 times, most recently from 64253f2 to a32737a Compare October 4, 2024 08:45
@Micket
Copy link
Contributor Author

Micket commented Oct 10, 2024

OK, so meson is really strange when specifying both buildtype and optmization/debug flags.
They claim that they mutually exclusive, but I disagree; the value of "buildtype" itself has value given things like

if get_option('buildtype') == 'release':
   # custom logic here

In particular -Db_ndebug=if-release is something a lot of things set (all of meson-python projects do this by default), so, it's not some little thing either. Though, if-release also leaves a lot to be desired; what about debugoptimized? is that supposed to be a release-like thing? Again debug is being used vaguely here.

I checked the meson.build from all the stuff that uses MesonNinja to see what, if any, such custom logic anyone might be doing, and apart from some harmless flags and b_ndebug=if-release, I couldn't find anything that worried me, so I'm going to go with the option of making buildtype and optimization/debug mutually exclusive for our easyblock as well, and just rely on optimization, debug, and b_ndebug flags.

@Micket
Copy link
Contributor Author

Micket commented Oct 10, 2024

Test report by @Micket

Overview of tested easyconfigs (in order)

  • SUCCESS ATK-2.38.0-GCCcore-13.2.0.eb
  • SUCCESS at-spi2-atk-2.38.0-GCCcore-13.2.0.eb
  • SUCCESS at-spi2-core-2.50.0-GCCcore-13.2.0.eb
  • SUCCESS cairo-1.18.0-GCCcore-13.3.0.eb
  • FAIL (build issue) cairomm-1.16.2-GCC-12.3.0.eb (partial log available at https://gist.github.com/Micket/21e0eee0053815efb7ebedf48fbbfef8)
  • SUCCESS desktop-file-utils-0.27-GCCcore-12.3.0.eb
  • SUCCESS DFT-D4-3.6.0-intel-2022a.eb
  • SUCCESS evince-45.0-GCC-12.3.0.eb
  • SUCCESS freebayes-1.3.7-gfbf-2023a-R-4.3.2.eb
  • SUCCESS FUSE-3.16.2-GCCcore-12.3.0.eb
  • SUCCESS Gdk-Pixbuf-2.42.10-GCCcore-13.2.0.eb
  • SUCCESS GLib-2.80.4-GCCcore-13.3.0.eb
  • SUCCESS GLibmm-2.78.1-GCCcore-13.2.0.eb
  • SUCCESS glib-networking-2.72.1-GCCcore-11.3.0.eb
  • SUCCESS GObject-Introspection-1.78.1-GCCcore-13.2.0.eb
  • SUCCESS Graphene-1.10.8-GCCcore-13.2.0.eb
  • SUCCESS GST-plugins-bad-1.22.5-GCC-12.3.0.eb
  • SUCCESS GST-plugins-base-1.22.5-GCC-12.3.0.eb
  • SUCCESS GStreamer-1.22.5-GCC-12.3.0.eb
  • SUCCESS GTK3-3.24.39-GCCcore-13.2.0.eb
  • SUCCESS GTK4-4.13.1-GCC-12.3.0.eb
  • SUCCESS inih-57-GCCcore-12.3.0.eb
  • SUCCESS libdrm-2.4.122-GCCcore-13.3.0.eb
  • SUCCESS libepoxy-1.5.10-GCCcore-13.2.0.eb
  • SUCCESS libGLU-9.0.3-GCCcore-13.2.0.eb
  • SUCCESS libglvnd-1.7.0-GCCcore-13.3.0.eb
  • SUCCESS libgxps-0.3.2-GCCcore-12.3.0.eb
  • SUCCESS libhandy-1.8.2-GCCcore-12.3.0.eb
  • SUCCESS libpciaccess-0.18.1-GCCcore-13.3.0.eb
  • SUCCESS LibSoup-3.0.8-GCC-11.3.0.eb
  • SUCCESS mm-common-1.0.6-GCCcore-13.2.0.eb
  • SUCCESS MODFLOW-6.4.4-foss-2023a.eb
  • SUCCESS Pango-1.51.0-GCCcore-13.2.0.eb
  • SUCCESS pixman-0.43.4-GCCcore-13.3.0.eb
  • SUCCESS PretextMap-0.1.9-GCCcore-12.3.0.eb
  • SUCCESS PyCairo-1.25.1-GCCcore-13.2.0.eb
  • SUCCESS Wayland-1.23.0-GCCcore-13.3.0.eb
  • SUCCESS wpebackend-fdo-1.14.1-GCCcore-11.3.0.eb
  • SUCCESS X11-20240607-GCCcore-13.3.0.eb
  • SUCCESS xtb-6.7.0-gfbf-2023a.eb
  • SUCCESS Xvfb-21.1.9-GCCcore-13.2.0.eb

Build succeeded for 40 out of 41 (41 easyconfigs in total)
vera-skylake-build - Linux Rocky Linux 8.9, x86_64, Intel Xeon Processor (Skylake, IBRS, no TSX), Python 3.6.8
See https://gist.github.com/Micket/49102596d81d8d925377b46e82b7475f for a full test report.

@Micket
Copy link
Contributor Author

Micket commented Oct 10, 2024

The error on cairomm is something unrelated, 2 tests crashes for me no matter what easyblock i use, even in 4.9.2, so it shouldn't let this block the merge.

Copy link
Member

@jfgrimm jfgrimm left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 66 to 75
@property
def optimization(self):
"""Optimization level"""
if self.toolchain.options.get('noopt', False):
return 0
elif self.toolchain.options.get('lowopt', False):
return 1
else:
return 2

Copy link
Member

Choose a reason for hiding this comment

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

wait, what if we have 'opt': True (as in, use -O3)?
surely this needs another elif?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@property
def optimization(self):
"""Optimization level"""
if self.toolchain.options.get('noopt', False):
return 0
elif self.toolchain.options.get('lowopt', False):
return 1
else:
return 2
@property
def optimization(self):
"""Optimization level"""
if self.toolchain.options.get('noopt', False):
return 0
elif self.toolchain.options.get('lowopt', False):
return 1
elif self.toolchain.options.get('opt', False):
return 3
else:
return 2

@jfgrimm jfgrimm merged commit 93cd948 into easybuilders:5.0.x Oct 11, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Nice-to-have
Development

Successfully merging this pull request may close these issues.

set a default buildtype in MesonNinja easyblock
3 participants