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

WIP: meson build #120

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

WIP: meson build #120

wants to merge 25 commits into from

Conversation

vtorri
Copy link

@vtorri vtorri commented May 2, 2024

No description provided.

Copy link

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

At a quick glance, I have a couple suggestions.

meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson.build Outdated
Comment on lines 345 to 351
pthread_dep = dependency('threads')
if pthread_dep.found()
config_h.set10('MYTHREAD_POSIX', true,
description: 'Define to 1 when using POSIX threads (pthreads).'
)
have_threads = true
endif

Choose a reason for hiding this comment

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

This doesn't make sense, because pthread_dep doesn't specify required: false and therefore it cannot ever fail to be found (regardless of whether meson always finds it successfully).

So the if block is nonsense and should just be unconditional.

have_threads is therefore true if get_option('threads') is.

src/common/meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
src/liblzma/meson.build Outdated Show resolved Hide resolved
src/liblzma/meson.build Outdated Show resolved Hide resolved
gnu_symbol_visibility : 'hidden',
include_directories : lzma_incdir,
install : true,
link_args : sys_windows ? '-Wl,--output-def,liblzma.def.in' : '',
Copy link
Member

Choose a reason for hiding this comment

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

--output-def is for GNU ld and perhaps also lld from LLVM. CMakeLists.txt uses this option behind if(NOT MSVC). With CMake, the package builds with both GCC and Clang/LLVM + MinGW-w64.

Copy link
Author

Choose a reason for hiding this comment

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

yes, I know, that's something that will be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment indicating that. It's hard to know if something is wrong by omission or as a TODO.

Copy link
Author

Choose a reason for hiding this comment

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

so, on Windows:

  • if get_id() returns 'msvc', i use the Windows linker option /def:liblzma.def.in (either link.exe, lld-link.exe or xilink.exe linkers)
  • otherwise -Wl,--output-def,liblzma.def.in
    is it ok for you ?

Copy link
Member

Choose a reason for hiding this comment

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

The existing build systems create the DEF file only with MinGW-w64-based toolchains. No one has asked for it in MSVC builds so perhaps it's not important there. If one is building with MSVC, one might be building everything with it and thus not need DEF files for anything.

],
default_options: [
'b_ndebug=if-release',
'c_std=c11',
Copy link
Member

Choose a reason for hiding this comment

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

Since Meson 1.3.0 it's possible to have a list. For example, c_std=gnu99,c11,c99 would prefer gnu99 but then try c11 before c99. I don't know if MSVC counts as C99 as it officially supports only C11, I think, perhaps due to C99 having a few mandatory features that are optional in C11.

GNU extensions are used when they are available, thus gnu99 can make sense. gnu99 vs. c99 also affect the predefined macros. On my system with GCC 13.2.1, -std=c99 defines __STRICT_ANSI__ while -std=gnu99 doesn't. It can then affect the behavior of system headers.

Copy link
Author

Choose a reason for hiding this comment

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

I know that some project leaders do not like to use the latest version of meson. If there is no problem for you to use meson >= 1.3, then ok.

Copy link
Member

Choose a reason for hiding this comment

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

I actually don't know right now which Meson (or muon) version is OK to require. I just get an impression that a single value doesn't work everywhere (if MSVC needs C11 mode). I haven't checked in detail how the c_std option in Meson exactly works.

Copy link

@eli-schwartz eli-schwartz May 2, 2024

Choose a reason for hiding this comment

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

https://github.com/mesonbuild/meson/blob/master/mesonbuild/compilers/c.py#L453-L485

For MSVC, meson ignores c99 and passes /std:XXX when the mode is c11 or c17. The difference is really about whether you want to opt into GNU extensions (which MSVC obviously doesn't support). Doing that might require bumping the minimum meson requirement, yes.

Copy link
Member

Choose a reason for hiding this comment

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

I vaguely remember that, a very long time ago, using gnu99 was better in practice when the intention was to use non-standard extensions (language or library) when they are available.

If gnu99 doesn't put MSVC into C11/C17 mode then MSVC needs some other c_std option. MSVC is a special case in so many ways that handling it specially here could be OK too, assuming it's easy to do.

Using gnu11 with GCC and Clang and strict C11 with compilers that don't support GNU extensions could be OK too. But if compiler doesn't support C11 then it should fallback to C99 if that is supported.

The list method that requires Meson >= 1.3.0 would make all this trivially easy. muon says it's currently at meson compat version 1.1.

So, oh well, maybe try gnu11 for now if that helps getting MSVC to C11 mode. This is a small detail in the big picture and can be worried more a little later.

Copy link
Author

Choose a reason for hiding this comment

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

what I did for muon with cl.exe compiler: I just pass the required standard. So, no problem with c11. (there will be a message from the compiler if the standard is not correct)
I maybe should pass the standard if it is only c11 or c17 like meson. I'll ask the author

Copy link
Member

Choose a reason for hiding this comment

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

The Meson docs give an impression that c_std=gnu11 would fallback to C11 with MSVC but this might become deprecated and eventually an error. So one would need to list the possible standards.

I think requiring new enough Meson and using c_std=gnu99,c11,c99 or c_std=gnu11,gnu99,c11,c99 is fine for now. It might not be muon compatible right now but I would expect this kind of feature to get implemented at some point in muon too. Very probably the first versions of Meson support won't be as portable as the Autotools build is and it doesn't need to be. Progress happens one step at a time.

@Larhzu
Copy link
Member

Larhzu commented May 2, 2024

The following is relevant only when other things have already been finished. I'm writing it here so that it won't be forgotten.

The tuklib files are a bit similar to Gnulib in sense that a subset of the files is meant to be reusable in other projects somewhat easily. With Autotools and CMake, one can copy the relevant m4/tuklib_*.m4 or cmake/tuklib_*.cmake files along with the .h and .c files.

Meson's language is intentionally more restrictive so it's OK that the first iterations put all tuklib checks into a single meson.build. This is not ideal in the long term though as it makes it difficult to reuse a subset of the modules.

One method would be to have a separate repository for the tuklib modules, each module having a meson.build snippet file. Then a script would take a list of modules to use and generate a single meson.build from the snippets (in the right order, handling possible dependencies). The downside is that each project using a different subset of the modules would end up with a different generated build system file in the source package (simpler than configure but still).

Another idea could be to have a variable, for example, tuklib_modules which would contain a list of modules to use. Top-level meson.build would set that variable and then do subdir(src/common/tuklib_meson) which would further do subdir(modulename123) for each module name that was found in tuklib_modules (in dependency order).

This way the checks of each module would be in their own meson.build files. In the parent directory the meson.build file would list all modules, including those that a specific project doesn't need. It would be just a boring chain of if-subdir-endif lines in the correct order to satisfy possible cross-module dependencies. This way no generated files would be needed to use a subset of the modules in another project.

(The if-subdir-endif file might still be a generated file in the module repository. However, it would be the same for all projects that use the modules, thus there wouldn't be as many versions of the generated file, making reviews easier.)

The set of tuklib modules is tiny so in this sense this is silly overthinking. But I'm curious how this would have to be done with bigger number of reusable modules (like Gnulib). It seems essential to split the per-module checks into separate files if there are tens of modules and different projects use a different set of modules. A per-project generated meson.build that contains checks for tens of modules sounds too similar to a generated configure. Avoiding such per-project generated files is a big reason why this Meson discussion in XZ Utils is happening in the first place.


Edited to add:

Modules may add build options too. Currently unaligned-access is listed in the top-level meson_options.txt which is OK for now but, like with meson.build files, it's not ideal in the long run as it would require project-specific generated files.

Or perhaps the solution to generated files is to have a mesongen.py which would generate the meson.build files and meson_options.txt. This would mean no muon support though as Python would be a hard dependency. But this way the package wouldn't ship with any generated build system files, and one would only need Python (which is already needed for Meson) to genete the files needed by Meson. The idea of a build system generator doesn't sound great though but if pure Meson solutions are inconvenient, it might be an option too.

endif
have_threads = true
else
pthread_dep = dependency('threads', required : false)
Copy link

Choose a reason for hiding this comment

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

This is incorrect usage. dependency('threads') will always "find" a thread dependency.

dependency('threads') isn't designed to check for pthreads. It just allows you to use whatever threading API is available with the selected toolchain, so you should also use it on the if sys_windows branch.

pthreads have to be detected separately, like with cc.check_header('pthread.h').

CC: @eli-schwartz

Copy link
Member

Choose a reason for hiding this comment

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

At glance this sounds odd. XZ Utils can use pthreads and Windows threads. C11 threads are available on some platforms too but XZ Utils doesn't support those for now (they don't seem to add any practical value for now).

MinGW-w64 supports pthreads and Windows threads, the latter being preferred. Very recent MSVC supports C11 threads in addition to Windows threads.

When using MinGW-w64 and Windows threads, one specifically wants to ensure that the pthreads flags are not used. Thus the Autotools and CMake builds don't add any threading related options to the build when using Windows threads.

Autotools build assumes that pthreads is available on non-Windows, thus the build fails, for example, on MINIX 3 where pthreads isn't available. One has to manually use --disable-threads. Perhaps I should fix it to show an error at configure time with a message that suggest --disable-threads. This would match what CMakeLists.txt already does. I don't want to it to silently disable threading support.

meson.build Outdated Show resolved Hide resolved
src/common/meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
# lzma/Makefile.inc
lzma_src += [
'lzma/lzma_encoder_presets.c'
]
Copy link

Choose a reason for hiding this comment

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

Only this line needs to be inside this if, the rest of the lines can be outside since their comparisons are a subset of this if.

Copy link
Author

Choose a reason for hiding this comment

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

not so simple: lzma/Makefile.inc is included if lzma1 is enabled, but in lzma/Makefile.inc there are some code if lzma2 is enabled

src/liblzma/meson.build Outdated Show resolved Hide resolved
# delta/Makefile.inc
lzma_src += [
'delta/delta_common.c'
]
Copy link

Choose a reason for hiding this comment

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

Same, only this line needs to be inside this if, the rest of the lines can be outside since their comparisons are a subset of this if.

Comment on lines 256 to 266
if have_encoder_simple_filters == true
lzma_src += [
'simple/simple_encoder.c'
]
endif

if have_decoder_simple_filters == true
lzma_src += [
'simple/simple_decoder.c'
]
endif
Copy link

Choose a reason for hiding this comment

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

These if statements do not have to nest inside this if statement

meson.build Outdated Show resolved Hide resolved
if ('lzma1' in get_option('decoders')) or ('lzma2' in get_option('decoders'))
lzma_src += [
'lz/lz_decoder.c'
]
endif

if ('lzma1' in get_option('encoders')) or ('lzma1' in get_option('decoders'))
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if things like this should be factored out into their own variable, like:

have_lzma1_encoder = ...

Copy link
Author

Choose a reason for hiding this comment

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

i wonder if lzma1 and lzma2 should be not an option, but just required, as it's the main purpose of xz. That would simplify the build a LOT...

Copy link
Member

Choose a reason for hiding this comment

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

As discussed on IRC, it's fine to make LZMA1 and LZMA2 mandatory. Having a way to disable encoder support entirely is useful though.

meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson.build Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
src/xzdec/meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
src/common/meson.build Outdated Show resolved Hide resolved
src/common/meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
src/common/meson.build Outdated Show resolved Hide resolved

config_h.set10('HAVE_VISIBILITY', cc.has_function_attribute('visibility'),
description: 'Define to 1 if the compiler supports simple visibility declarations.'
)
Copy link
Member

Choose a reason for hiding this comment

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

@eli-schwartz: Elsewhere there is gnu_symbol_visibility: 'hidden'. Is it possible to use the result of that here somehow? I remember that on some platforms the attributes are accepted but produce compiler warnings as they aren't truly supported. I don't know if that is a case on any modern platform though (Cygwin was like that many years ago but I haven't tested in years).

Gnulib's visibility.m4 checks that both attribute and compiler option are supported using -Werror to enable visibility support. Meson's documentation doesn't tell if compiler.has_function_attribute() accepts the attribute if using it produces warnings but compiling still succeeds.

It's also a bit unfortunate if visibility support requires an option in both library and a separate check for the attribute support. Perhaps the gnu_symbol_visibility option could insert preprocessor macros like MESON_GNU_SYMBOL_VISIBILITY_HIDDEN __attribute__((__visibility("hidden"))) which the C/C++ code could use. I'm not sure what is the best way as perhaps this idea has downsides. And modern compilers have __has_attribute anyway.

Choose a reason for hiding this comment

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

Meson internally handles -Werror for exactly that reason:
.https://github.com/mesonbuild/meson/blob/7d28ff29396f9d7043204de8ddc52226b9903811/mesonbuild/compilers/mixins/clike.py#L1309-L1324

Most projects I have seen, actually check for __GNUC__ instead of using visibility.m4 even with autotools. This is kind of necessary anyway since you need preprocessor logic to figure out whether Microsoft dllexport/dllimport is viable, and visibility.m4 is ultimately not all that helpful overall.

Certain versions of... SunPro, if I recall correctly, are the only commonly known compiler that handles visibility but isn't either __GNUC__ or MSC_VER, and they use a third syntax instead?

Copy link
Member

Choose a reason for hiding this comment

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

Meson internally handles -Werror for exactly that reason:

Great! Documenting it could be nice to give more confidence. Or perhaps it's that coming from other build systems one is constantly suspecting that corner cases aren't handled well enough, sorry.

Most projects I have seen, actually check for __GNUC__ instead of using visibility.m4 even with autotools.

liblzma checks in this order in the C code:

  1. #if defined(_WIN32) || defined(__CYGWIN__) then use __declspec(dllexport).

  2. #if HAVE_VISIBILITY to use GNU C attributes. Checking for __GNUC__ wouldn't work as it can be defined on platforms that don't support the visibility attributes; one has to know if the attribute is actually supported.

  3. Otherwise assume no visibility support.

Certain versions of... SunPro, if I recall correctly, are the only commonly known compiler that handles visibility but isn't either __GNUC__ or MSC_VER, and they use a third syntax instead?

My note in b0d1422 says that SunPro (or whatever its name is this year) supports the GNU C attribute since 2007. At least modern releases support the -fvisibility option too. I didn't research when the command line option was added (it would make sense to add it when the attribute was but I don't know if that happened). The original option is -xldscope but I guess there's no point to add support for it when -fvisibility works too.

Copy link
Member

Choose a reason for hiding this comment

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

This

__attribute__((dllexport)) int foo(int x) { return x; }

results in a warning with GCC 14.1.1 on x86-64 GNU/Linux:

warning: ‘dllexport’ attribute directive ignored [-Wattributes]
    1 | __attribute__((dllexport)) int foo(int x) { return x; }
      | ^~~~~~~~~~~~~

But cc.has_function_attribute('dllexport') is still true. So -Werror isn't used or Meson shortcircuits the attribute check somehow. Thus I fear it might do the same with the visibility attribute as well.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry, I'm wrong. I just fell into the exact trap of #undef vs. #define being too easy to mix in config.h. So the previous message is noise. Sorry.

fast_unaligned_access = false
if get_option('unaligned-access').auto()
unaligned_access_cpus = [
'ppc',
Copy link
Member

@Larhzu Larhzu May 16, 2024

Choose a reason for hiding this comment

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

The Autoconf and CMake builds exclude 32-bit little endian PowerPC from fast unaligned access as I'm not sure how it behaves, especially with 64-bit values. I suppose it's an uncommon target anyway so better play it safe.

config_h.set10('HAVE___BUILTIN_ASSUME_ALIGNED', true,
description: 'Define to 1 if the GNU C extension __builtin_assume_aligned is supported.'
)
endif
Copy link
Member

Choose a reason for hiding this comment

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

__builtin_assume_aligned is only needed when unsafe type-punning is not used, thus this condition is upside down. It's better to check for the builtin in all cases.

Without unsafe type punning, __builtin_assume_aligned is important on strict-align archs because it is the only way (that I know) to read aligned integers fast from byte buffers without breaking strict aliasing rules. (Casting via union isn't possible when a function only gets a pointer to a byte buffer.)

I know that __has_builtin exists but it's still a fairly new feature and thus not useful in XZ Utils for now. __builtin_assume_aligned is a much older feature.

meson_options.txt Outdated Show resolved Hide resolved
meson_options.txt Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
@Larhzu
Copy link
Member

Larhzu commented May 17, 2024 via email

@Larhzu
Copy link
Member

Larhzu commented May 17, 2024 via email

@vtorri
Copy link
Author

vtorri commented May 18, 2024

(which generates #undef WORDS_BIGENDIAN on little endian) good for you ?
Yes. Thanks!

done

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.

5 participants