Issue 29527808: Noissue - Use meson to build the C++ code (Closed)
|
Created:
Aug. 25, 2017, 5:17 p.m. by hub Modified:
Dec. 15, 2017, 7:47 p.m. CC:
Sebastian Noack Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionNoissue - Use meson to build the C++ code
Patch Set 1 #Patch Set 2 : npm test now works #Patch Set 3 : Rebased. Now compile output to the source directory #
Total comments: 15
Patch Set 4 : Reworked. Now build source files one by one. #
Total comments: 16
Patch Set 5 : Now build objects one by one, with dependencies, and share them. #
Total comments: 13
Patch Set 6 : Last bits of feedback #
Total comments: 13
Patch Set 7 : Updated from feedback. now require meson 0.40.0, but no default C++ compiler #
Total comments: 3
Patch Set 8 : Properly build with DEBUG #
Total comments: 6
Patch Set 9 : Pin meson version. Deprecate compile #
Total comments: 4
Patch Set 10 : Removed compile. meson > 0.40.0 #
MessagesTotal messages: 29
not really for a review but more for a "here is what I came up with". I shall remove the `compile` script. And improve the documentation. Few things need polishing: - deal with the env for emscripten. It doesn't seem to be trivial in meson - we must have a separate build dir (no choice). I'm not against that at all since I do like generated files to be elsewhere, but this require some adjustments to be done in order to have the things work.
I believe Eric's CMake build system it better. meson has serious limitations like the impossibility to set the compiler to something else without setting the environment. The current version does not handle dependencies and is therefor IMHO clunky.
I actually find this approach nicer than CMake, Meson build files seem to be more flexible while still easy to read. At least reading Emscripten configuration can be done nicely as indicated below. Also, initial CMake call requiring a special environment is very awkward - here this is solved nicely because the initial call is inherently different, we are calling meson rather than ninja. This seems to do everything that the current compile script does while also not repeating unnecessary steps, especially with separate object generation I outlined below. Dependency tracking is indeed an issue however, we need to solve that without requiring dependencies to be configured manually. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode1 meson.build:1: project('adblockpluscore', license: ['GPL3']) Meson 0.39.1 (what you currently get on Ubuntu) complains: "Not enough arguments to project(). Needs at least the project name and one language" I guess that you have a newer version installed, but adding 'cpp' parameter won't hurt. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode4 meson.build:4: nodejs = find_program('node') I tried reading Emscripten configuration and the following worked (note that meson depends on Python3 so it doesn't introduce an additional dependency): python = import('python3').find_python() emscripten_config = get_option('emscripten-config') command = run_command(python, '-c', 'import os.path, sys;print(os.path.expanduser(sys.argv[1]))', emscripten_config) if command.returncode() != 0 error(command.stderr().strip()) endif emscripten_config = command.stdout().strip() message('Emscripten config file: ' + emscripten_config) command = run_command(python, '-c', 'import sys;exec(open(sys.argv[1]).read());print(EMSCRIPTEN_ROOT)', emscripten_config) if command.returncode() != 0 error(command.stderr().strip()) endif emcc = join_paths(command.stdout().strip(), 'emcc') emcc = find_program(emcc) command = run_command(python, '-c', 'import sys;exec(open(sys.argv[1]).read());print(NODE_JS)', emscripten_config) if command.returncode() != 0 error(command.stderr().strip()) endif nodejs = find_program(command.stdout().strip(), 'node', 'nodejs') https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode76 meson.build:76: generation_args += param[0] + '=' + '@0@'.format(param[1]) Nit: You should really unify leading whitespace in this file, we should use two spaces for indentation. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode93 meson.build:93: output: COMPILER_OUTPUT, I'd consider that a temporary hack. It's probably best to have compiled.js generated in the build/ directory and adjust require() paths accordingly. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode94 meson.build:94: command: [emcc, '-o', output_file, '-std=c++1z', -std=c++1z parameter is unnecessary here, it is already part of ADDITIONAL_PARAMS. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode97 meson.build:97: ADDITIONAL_PARAMS) Object files should really be generated as separate targets, to make the builds faster. If I make sources a list of strings rather than files, the following works for me: compiler_args = defines_args + generation_args + optional_args + ADDITIONAL_PARAMS objects = [] foreach source_file : sources output_file = source_file + '.o' objects += custom_target(output_file, input: files(source_file), output: source_file.underscorify() + '.o', command: [emcc, '-c', '-o', '@OUTPUT@', '@INPUT@'] + compiler_args) endforeach output_file = meson.source_root() + '/lib/' + COMPILER_OUTPUT compiler_output = custom_target('compiled.js', build_by_default: true, input: objects, output: COMPILER_OUTPUT, command: [emcc, '-o', output_file, '--post-js', bindings_output, '@INPUT@'] + compiler_args) https://codereview.adblockplus.org/29527808/diff/29532724/meson_options.txt File meson_options.txt (right): https://codereview.adblockplus.org/29527808/diff/29532724/meson_options.txt#n... meson_options.txt:1: Nit: pointless blank line
With all the suggestions, this looks better. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode1 meson.build:1: project('adblockpluscore', license: ['GPL3']) On 2017/10/11 10:00:47, Wladimir Palant wrote: > Meson 0.39.1 (what you currently get on Ubuntu) complains: "Not enough arguments > to project(). Needs at least the project name and one language" > > I guess that you have a newer version installed, but adding 'cpp' parameter > won't hurt. Done. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode4 meson.build:4: nodejs = find_program('node') On 2017/10/11 10:00:47, Wladimir Palant wrote: > I tried reading Emscripten configuration and the following worked (note that > meson depends on Python3 so it doesn't introduce an additional dependency): > > python = import('python3').find_python() > > emscripten_config = get_option('emscripten-config') > command = run_command(python, '-c', 'import os.path, > sys;print(os.path.expanduser(sys.argv[1]))', emscripten_config) > if command.returncode() != 0 > error(command.stderr().strip()) > endif > emscripten_config = command.stdout().strip() > message('Emscripten config file: ' + emscripten_config) > > command = run_command(python, '-c', 'import > sys;exec(open(sys.argv[1]).read());print(EMSCRIPTEN_ROOT)', emscripten_config) > if command.returncode() != 0 > error(command.stderr().strip()) > endif > emcc = join_paths(command.stdout().strip(), 'emcc') > emcc = find_program(emcc) > > command = run_command(python, '-c', 'import > sys;exec(open(sys.argv[1]).read());print(NODE_JS)', emscripten_config) > if command.returncode() != 0 > error(command.stderr().strip()) > endif > nodejs = find_program(command.stdout().strip(), 'node', 'nodejs') Done. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode76 meson.build:76: generation_args += param[0] + '=' + '@0@'.format(param[1]) On 2017/10/11 10:00:47, Wladimir Palant wrote: > Nit: You should really unify leading whitespace in this file, we should use two > spaces for indentation. Done. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode93 meson.build:93: output: COMPILER_OUTPUT, On 2017/10/11 10:00:47, Wladimir Palant wrote: > I'd consider that a temporary hack. It's probably best to have compiled.js > generated in the build/ directory and adjust require() paths accordingly. Acknowledged. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode94 meson.build:94: command: [emcc, '-o', output_file, '-std=c++1z', On 2017/10/11 10:00:47, Wladimir Palant wrote: > -std=c++1z parameter is unnecessary here, it is already part of > ADDITIONAL_PARAMS. Done. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode97 meson.build:97: ADDITIONAL_PARAMS) On 2017/10/11 10:00:47, Wladimir Palant wrote: > Object files should really be generated as separate targets, to make the builds > faster. I wanted to use the c++ targets, but meson doesn't allow overriding the compiler - expect by passing environment, which we don't want to have too. It is a feature according to them. Your suggestion make sense. https://codereview.adblockplus.org/29527808/diff/29532724/meson_options.txt File meson_options.txt (right): https://codereview.adblockplus.org/29527808/diff/29532724/meson_options.txt#n... meson_options.txt:1: On 2017/10/11 10:00:48, Wladimir Palant wrote: > Nit: pointless blank line Done.
I wonder whether creating a tiny Python wrapper around the meson command would be worth it - that wrapper could take over reading Emscripten configuration, setting CXX environment variable and then running meson with its own parameters. We might get a much nicer build file then. In particular, I would like to reuse object files between bindings and the actual output - but with the current approach it would cause too much code duplication. https://codereview.adblockplus.org/29527808/diff/29532724/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode97 meson.build:97: ADDITIONAL_PARAMS) On 2017/10/11 18:23:16, hub wrote: > I wanted to use the c++ targets, but meson doesn't allow overriding the compiler That's too bad. It would have been much nicer if we could tell to use emcc like the regular clang. https://codereview.adblockplus.org/29527808/diff/29573838/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29573838/meson.build#newcode1 meson.build:1: project('adblockpluscore', 'cpp', license: ['GPL3']) I suspect that the build will now fail if no C++ compiler is found on the system. If that's the case, requiring a newer meson version should be the lesser evil... https://codereview.adblockplus.org/29527808/diff/29573838/meson.build#newcode137 meson.build:137: ] + compiler_args) I looked into dependencies generation and we need to specify a depsfile parameter here as well as pass -MD and -MF parameters to emcc so that this file is actually generated: objects += custom_target(output_file, input: files(source_file), output: output_file, depfile: output_file + '.deps', command: [ emcc, '-MD', '-MF', '@DEPFILE@', '-c', '-o', '@OUTPUT@', '@INPUT@' ] + compiler_args) We would need the same thing for the bindings generator however.
Alternatively, we could have a script generate a cross-compiling configuration that could then be passed to Emscripten via command line parameter. This might be a cleaner approach.
https://codereview.adblockplus.org/29527808/diff/29573838/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29573838/README.md#newcode30 README.md:30: mkdir build This step is unnecessary, meson will create the directory automatically. https://codereview.adblockplus.org/29527808/diff/29573838/README.md#newcode31 README.md:31: meson build By default, a debug build will be created. So we should probably add `--buildtype debug|release` here. https://codereview.adblockplus.org/29527808/diff/29573838/README.md#newcode36 README.md:36: ninja We should mention that running meson is normally a one-time thing, ninja will detect changes to build configuration automatically. https://codereview.adblockplus.org/29527808/diff/29573838/README.md#newcode50 README.md:50: `build/compiled.js` without the `PRINT_BINDINGS` symbol, so that the It's still lib/compiled.js right now. We should update this section so that it no longer mentions PRINT_BINDINGS symbol. That's a separate issue however. https://codereview.adblockplus.org/29527808/diff/29573838/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29573838/meson.build#newcode131 meson.build:131: output_file = source_file.underscorify() + '.o' Nit: You have three spaces indentation here. https://codereview.adblockplus.org/29527808/diff/29573838/meson.build#newcode137 meson.build:137: ] + compiler_args) Actually, I succeeded reusing objects between bindings and main application without duplicating too much code. For that I had to split out '-fno-exceptions' from ADDITIONAL_PARAMS into CORE_PARAMS, this flag cannot be set for bindings generator. I also renamed sources variable into shared_sources. The object generation looks like this: # build objects. core_objects = [] bindings_objects = [] shared_objects = [] foreach group : ['core', 'bindings', 'shared'] objects = [] foreach source_file : get_variable(group + '_sources') output_file = source_file.underscorify() + '.o' command = [ emcc, '-MD', '-MF', '@DEPFILE@', '-c', '-o', '@OUTPUT@', '@INPUT@' ] + compiler_args if group != 'bindings' command += CORE_PARAMS endif objects += custom_target(output_file, input: files(source_file), output: output_file, depfile: output_file + '.deps', command: command) endforeach # Ideally, we would call set_variable() here but that doesn't work with arrays # (see https://github.com/mesonbuild/meson/issues/1481). So we have to do this # shifting dance instead. core_objects = bindings_objects bindings_objects = shared_objects shared_objects = objects endforeach The bindings-generator target has to be moved below this code then and work on bindings_objects + shared_objects. The '-std=c++1z' parameter is no longer needed there. And the compiled.js target works on core_objects + shared_objects.
https://codereview.adblockplus.org/29527808/diff/29573838/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29573838/README.md#newcode30 README.md:30: mkdir build On 2017/10/12 08:46:09, Wladimir Palant wrote: > This step is unnecessary, meson will create the directory automatically. Done. https://codereview.adblockplus.org/29527808/diff/29573838/README.md#newcode31 README.md:31: meson build On 2017/10/12 08:46:09, Wladimir Palant wrote: > By default, a debug build will be created. So we should probably add > `--buildtype debug|release` here. Done. https://codereview.adblockplus.org/29527808/diff/29573838/README.md#newcode36 README.md:36: ninja On 2017/10/12 08:46:08, Wladimir Palant wrote: > We should mention that running meson is normally a one-time thing, ninja will > detect changes to build configuration automatically. Done. https://codereview.adblockplus.org/29527808/diff/29573838/README.md#newcode50 README.md:50: `build/compiled.js` without the `PRINT_BINDINGS` symbol, so that the On 2017/10/12 08:46:08, Wladimir Palant wrote: > It's still lib/compiled.js right now. > > We should update this section so that it no longer mentions PRINT_BINDINGS > symbol. That's a separate issue however. Done. https://codereview.adblockplus.org/29527808/diff/29573838/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29573838/meson.build#newcode131 meson.build:131: output_file = source_file.underscorify() + '.o' On 2017/10/12 08:46:09, Wladimir Palant wrote: > Nit: You have three spaces indentation here. Done. https://codereview.adblockplus.org/29527808/diff/29573838/meson.build#newcode137 meson.build:137: ] + compiler_args) On 2017/10/11 21:01:23, Wladimir Palant wrote: > I looked into dependencies generation and we need to specify a depsfile > parameter here as well as pass -MD and -MF parameters to emcc so that this file > is actually generated: > > objects += custom_target(output_file, > input: files(source_file), > output: output_file, > depfile: output_file + '.deps', > command: [ > emcc, '-MD', '-MF', '@DEPFILE@', '-c', > '-o', '@OUTPUT@', '@INPUT@' > ] + compiler_args) > > We would need the same thing for the bindings generator however. Done. https://codereview.adblockplus.org/29527808/diff/29573838/meson.build#newcode137 meson.build:137: ] + compiler_args) On 2017/10/11 21:01:23, Wladimir Palant wrote: > I looked into dependencies generation and we need to specify a depsfile > parameter here as well as pass -MD and -MF parameters to emcc so that this file > is actually generated: > > objects += custom_target(output_file, > input: files(source_file), > output: output_file, > depfile: output_file + '.deps', > command: [ > emcc, '-MD', '-MF', '@DEPFILE@', '-c', > '-o', '@OUTPUT@', '@INPUT@' > ] + compiler_args) > > We would need the same thing for the bindings generator however. Done.
I am much more optimistic about using meson now - still quirky but it actually works nicely. Thanks to Wladimir.
A few minor issues but altogether this is looking good. https://codereview.adblockplus.org/29527808/diff/29574659/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29574659/README.md#newcode33 README.md:33: make a release build. Nit: make => create on both occasions? Seems to sound better to me. https://codereview.adblockplus.org/29527808/diff/29574659/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode76 meson.build:76: core_sources = [ 'compiled/traceInit.cpp' ] Nit: I'd list the file name here on a separate line and with a trailing comma, for consistency and for the case that we might add more in future. https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode95 meson.build:95: debug = get_option('buildtype') Nit: This variable should be named buildtype or mode. "debug" is merely one possible value for it. https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode117 meson.build:117: command += CORE_PARAMS I think we should add optional args only here and in compiled.js target, bindings generator with tracing enabled won't run under Node. https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode142 meson.build:142: files('compiled/bindings/library.js'), Does this detect compiled/bindings/library.js as a dependency automatically? We definitely need to indicate that JS_LIBRARY is a dependency. Also a nit: we should use join_paths() here and probably declare this as a BINDINGS_JS_LIBRARY constant for consistency. https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode146 meson.build:146: bindings_output = custom_target('bindings-output', Nit: Use BINDINGS_OUTPUT for target name? https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode154 meson.build:154: compiler_output = custom_target('compiled.js', Nit: Use COMPILER_OUTPUT for target name?
https://codereview.adblockplus.org/29527808/diff/29574659/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29574659/README.md#newcode33 README.md:33: make a release build. On 2017/10/13 09:45:39, Wladimir Palant wrote: > Nit: make => create on both occasions? Seems to sound better to me. Done. https://codereview.adblockplus.org/29527808/diff/29574659/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode76 meson.build:76: core_sources = [ 'compiled/traceInit.cpp' ] On 2017/10/13 09:45:40, Wladimir Palant wrote: > Nit: I'd list the file name here on a separate line and with a trailing comma, > for consistency and for the case that we might add more in future. Done. https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode95 meson.build:95: debug = get_option('buildtype') On 2017/10/13 09:45:39, Wladimir Palant wrote: > Nit: This variable should be named buildtype or mode. "debug" is merely one > possible value for it. Done. https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode142 meson.build:142: files('compiled/bindings/library.js'), On 2017/10/13 09:45:39, Wladimir Palant wrote: > Does this detect compiled/bindings/library.js as a dependency automatically? We > definitely need to indicate that JS_LIBRARY is a dependency. I wasn't sure, but it actually does. I checked. > Also a nit: we should use join_paths() here and probably declare this as a > BINDINGS_JS_LIBRARY constant for consistency. Done. https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode146 meson.build:146: bindings_output = custom_target('bindings-output', On 2017/10/13 09:45:39, Wladimir Palant wrote: > Nit: Use BINDINGS_OUTPUT for target name? Done. https://codereview.adblockplus.org/29527808/diff/29574659/meson.build#newcode154 meson.build:154: compiler_output = custom_target('compiled.js', On 2017/10/13 09:45:40, Wladimir Palant wrote: > Nit: Use COMPILER_OUTPUT for target name? Done.
LGTM As far as I am concerned, this can land. However, I'd like to hear other people's opinion on using meson here, in particularly Sergei's. Also, I'd like Sebastian's feedback because we'll need to integrate this with adblockpluschrome's build system in the long term.
On 2017/10/11 21:01:24, Wladimir Palant wrote: > I wonder whether creating a tiny Python wrapper around the meson command would > be worth it - that wrapper could take over reading Emscripten configuration, > setting CXX environment variable and then running meson with its own parameters. > We might get a much nicer build file then. In particular, I would like to reuse > object files between bindings and the actual output - but with the current > approach it would cause too much code duplication. I have a similar opinion and in addition I think about mirroring it and putting into third_party of a project. I would even propose to consider the option to not use meson syntax but rather to use it as a python module despite http://mesonbuild.com/FAQ.html#why-is-meson-not-just-a-python-module-so-i-cou.... In general meson seems OK, though there are some corners, let's see how it goes. The codereview is also good, I don't mind to have it committed, I guess there will be more work anyway. https://codereview.adblockplus.org/29527808/diff/29573838/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29573838/meson.build#newcode1 meson.build:1: project('adblockpluscore', 'cpp', license: ['GPL3']) On 2017/10/11 21:01:23, Wladimir Palant wrote: > I suspect that the build will now fail if no C++ compiler is found on the > system. If that's the case, requiring a newer meson version should be the lesser > evil... Yes, it fails if it cannot find any C++ compiler, e.g. it does not work on Windows. https://codereview.adblockplus.org/29527808/diff/29575688/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29575688/README.md#newcode23 README.md:23: * [ninja](https://www.ninja-build.org) It seems it would be good to specify minimal versions of both meson and ninja because there are new features and bugfixes in newer versions which can be essential for us. https://codereview.adblockplus.org/29527808/diff/29575688/README.md#newcode23 README.md:23: * [ninja](https://www.ninja-build.org) Just a notice, maybe add some tips about installing them, e.g. meson can be installed using pip and ninja package is usually actually called ninja-build, ninja is unrelated package. https://codereview.adblockplus.org/29527808/diff/29575688/README.md#newcode32 README.md:32: By default it will create a debug build. Pass `--buildtype release` to On 2017/10/12 18:12:11, hub wrote: > On 2017/10/12 08:46:09, Wladimir Palant wrote: > > By default, a debug build will be created. So we should probably add > > `--buildtype debug|release` here. > > Done. Actually, IMO one should follow some convention distinguishing debug and release directories, e.g. meson build/release --builttype=release ninja -C build/release. https://codereview.adblockplus.org/29527808/diff/29575688/README.md#newcode38 README.md:38: ninja What about ninja -C build? https://codereview.adblockplus.org/29527808/diff/29575688/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29575688/meson.build#newcode1 meson.build:1: project('adblockpluscore', 'cpp', license: ['GPL3']) I wonder how we plan to use it in the derived projects. In particular, there is a concept of subprojects, should we leverage it? https://codereview.adblockplus.org/29527808/diff/29575688/meson.build#newcode87 meson.build:87: foreach define : DEFINES DEFINES is empty, it seems a good idea to keep it, though it's a dead code. https://codereview.adblockplus.org/29527808/diff/29575688/meson.build#newcode100 meson.build:100: optional_args += '-g3' We need to check why -g3 is added twice. https://codereview.adblockplus.org/29527808/diff/29575688/meson.build#newcode107 meson.build:107: compiler_args = defines_args + generation_args + optional_args + ADDITIONAL_PARAMS optional_args are included two times, the second time on the lines 118 and 166. https://codereview.adblockplus.org/29527808/diff/29575688/meson.build#newcode155 meson.build:155: output_file = join_paths(meson.source_root(), 'lib', COMPILER_OUTPUT) On 2017/10/11 10:00:47, Wladimir Palant wrote: https://codereview.adblockplus.org/29527808/diff/29532724/meson.build#newcode93 > I'd consider that a temporary hack. It's probably best to have compiled.js > generated in the build/ directory and adjust require() paths accordingly. Can we put a #TODO: here? Optionally include a reminder to update README.md (currently it's line 42).
On 2017/10/11 10:00:53, Wladimir Palant wrote: > Also, initial CMake call > requiring a special environment is very awkward - here this is solved nicely > because the initial call is inherently different, we are calling meson rather > than ninja. This reflects a misunderstanding of how CMake operates. CMake and meson are essentially the same here. The first call to CMake, just as with meson, creates build-files for a build-specific tool. With meson, this tool is always ninja. With CMake, it's any of the supported generators, including ninja, but also including make (in several different varieties), and several IDE's with their internal build systems. In both cases, subsequent calls are to the build tool, whatever it be. In addition, with CMake it's possible to have CMake call the build tool with `cmake --build`. The README I posted is not a CMake tutorial, and I appreciate that this isn't obvious. The advantage of CMake here is that no one needs to install a build tool; there's (almost) always one already installed on the system. This removes an external dependency. There's also a bit of misunderstanding about this "special environment" required for CMake/Emscripten. This is not an essential deficiency with CMake, but rather a problem with the present version of the toolchain file that emscripten ships with. The current version requires being run from an Emscripten shell; that's all the environment is, and the Emscripten SDK has the requisite scripts already. The present meson script reads directly from the configuration file. It would be possible to have CMake work analogously.
On 2017/10/13 18:54:06, Wladimir Palant wrote: > However, I'd like to hear other > people's opinion on using meson here, in particularly Sergei's. I believe that tool selection depends more on decision scope, requirements and maturity of the tool than on narrow technical concerns. 1) Decision Scope. The present review and mine for CMake started as an effort to supplant `gyp`, which has been deprecated by Google. We're currently using `gyp` in libadblockplus. The scope of the decision, therefore, should not be only about this immediate issue of compiling with Emscripten in adblockpluscore, but on the larger development effort, including libadblockplus certainly but also other projects. From what I can tell, we started using `gyp` because that's what v8 was using to build, so it made it easier to treat v8 as a third-party dependency from within libadblockplus. For a number of interlocking reasons, libadblockplus is moving to treat v8 as a pre-built external dependency. One reason is for CI integration. Another is to support other engines, since v8 is going in a direction not well-suited for "small" Android devices. It has become a non-requirement to use whatever v8 is using, and we'll need to integrate with whatever it's doing separately. 2) Requirements. Do we have a requirement that we support generation of IDE projects or not? `gyp` does this well, and if we drop this requirement, we'll have a regression in capability. Meson does not do this, nor do they have any intention of doing so. From http://mesonbuild.com/IDE-integration.html: "Meson has exporters for Visual Studio and XCode, but writing a custom backend for every IDE out there is not a scalable approach." Instead, meson is relying upon IDE authors to make the effort of integrating Meson. This may or may not happen in time, but it's certainly not present yet. It's not even present yet for Eclipse, insofar as I can tell; all I can find there is an editor for meson build files. If we don't support IDE generation, what that means is that each individual developer will need to set up a personal environment separately, a needless duplication of effort. Personally, I can't imagine going back in time to not using an IDE; my productivity loss would be unacceptable. 3) Maturity. We are already in our present situation, needing to rewrite our build, because we choose a novel tool, `gyp`, that was orphaned after a few years. The question here is one of risk management. Will we incur a safe dependency or a risky one? Very simply: CMake is mature, meson is not. Meson might become mature over time, but it's not there yet. Meson may succeed, or it may wither; time will tell. CMake, on the other hand, is certainly going to be around for quite a while. LLVM, upon which Emscripten is based, uses CMake. If LLVM switches build tools, it would be cause to reexamine use of CMake. (Note: we have no *technical* need to use CMake just because LLVM is using it.) 4) Technical merit. 4a) Domain-specific language for builds. Both CMake and meson are, in broad categorical terms, the same kind of thing. They both use an imperative language to set up an internal model of a build and then output build files. (In my view it would be an improvement to have a pure dependency model and a declarative language for it, but that's not the state of technology.) The CMake language is definitely showing its age. It's not great, but it is adequate. There's also a lot in it to handle many possibilities, a lot of which we won't need, some of which we likely will. The Meson language is more modern and cleaner. It has out-of-the-box support for fewer capabilities because of its relatively young age. 4b) Documentation The CMake documentation can be downright maddening. Those who have used both say that the meson documentation is much better. ---------- I am in favor of CMake because of its maturity and lower risk. I do acknowledge that CMake does not dominate meson in comparison, as meson is superior in certain ways. Nonetheless, the balance for me favors CMake.
On 2017/10/20 16:54:39, Eric wrote: > On 2017/10/13 18:54:06, Wladimir Palant wrote: > > However, I'd like to hear other > > people's opinion on using meson here, in particularly Sergei's. > > I believe that tool selection depends more on decision scope, requirements and > maturity of the tool than on narrow technical concerns. > > 1) Decision Scope. The present review and mine for CMake started as an effort to > supplant `gyp`, which has been deprecated by Google. We're currently using `gyp` > in libadblockplus. The scope of the decision, therefore, should not be only > about this immediate issue of compiling with Emscripten in adblockpluscore, but > on the larger development effort, including libadblockplus certainly but also > other projects. > > From what I can tell, we started using `gyp` because that's what v8 was using to > build, so it made it easier to treat v8 as a third-party dependency from within > libadblockplus. For a number of interlocking reasons, libadblockplus is moving > to treat v8 as a pre-built external dependency. One reason is for CI > integration. Another is to support other engines, since v8 is going in a > direction not well-suited for "small" Android devices. It has become a > non-requirement to use whatever v8 is using, and we'll need to integrate with > whatever it's doing separately. > > 2) Requirements. Do we have a requirement that we support generation of IDE > projects or not? `gyp` does this well, and if we drop this requirement, we'll > have a regression in capability. Meson does not do this, nor do they have any > intention of doing so. From http://mesonbuild.com/IDE-integration.html: > "Meson has exporters for Visual Studio and XCode, but writing a custom > backend for every IDE out there is not a scalable approach." > Instead, meson is relying upon IDE authors to make the effort of integrating > Meson. This may or may not happen in time, but it's certainly not present yet. > It's not even present yet for Eclipse, insofar as I can tell; all I can find > there is an editor for meson build files. > > If we don't support IDE generation, what that means is that each individual > developer will need to set up a personal environment separately, a needless > duplication of effort. Personally, I can't imagine going back in time to not > using an IDE; my productivity loss would be unacceptable. > > 3) Maturity. We are already in our present situation, needing to rewrite our > build, because we choose a novel tool, `gyp`, that was orphaned after a few > years. The question here is one of risk management. Will we incur a safe > dependency or a risky one? > > Very simply: CMake is mature, meson is not. Meson might become mature over time, > but it's not there yet. Meson may succeed, or it may wither; time will tell. > CMake, on the other hand, is certainly going to be around for quite a while. > LLVM, upon which Emscripten is based, uses CMake. If LLVM switches build tools, > it would be cause to reexamine use of CMake. (Note: we have no *technical* need > to use CMake just because LLVM is using it.) > > 4) Technical merit. > > 4a) Domain-specific language for builds. Both CMake and meson are, in broad > categorical terms, the same kind of thing. They both use an imperative language > to set up an internal model of a build and then output build files. (In my view > it would be an improvement to have a pure dependency model and a declarative > language for it, but that's not the state of technology.) > > The CMake language is definitely showing its age. It's not great, but it is > adequate. There's also a lot in it to handle many possibilities, a lot of which > we won't need, some of which we likely will. The Meson language is more modern > and cleaner. It has out-of-the-box support for fewer capabilities because of its > relatively young age. > > 4b) Documentation > > The CMake documentation can be downright maddening. Those who have used both say > that the meson documentation is much better. > > ---------- > > I am in favor of CMake because of its maturity and lower risk. I do acknowledge > that CMake does not dominate meson in comparison, as meson is superior in > certain ways. Nonetheless, the balance for me favors CMake. Eric, thanks for the points, could you also put the arguments to the document on gdrive (Shared docs > Development > Build systems)?
On 2017/10/20 15:20:04, Eric wrote: > This reflects a misunderstanding of how CMake operates. CMake and meson are > essentially the same here. The first call to CMake, just as with meson, creates > build-files for a build-specific tool. No misunderstanding here, it's merely more confusing with CMake - one has to understand that the two calls to CMake are completely different. > The present meson script reads directly from the configuration file. It > would be possible to have CMake work analogously. Yes, I understand that. But so far the main advantage of CMake appears to be the fact that Emscripten provides a toolchain for it. If it isn't really usable (or overly complicated to use) then this advantage is void. But Sergei is right of course, this isn't a discussion for a review.
just so that we have the latest iteration here. https://codereview.adblockplus.org/29527808/diff/29575688/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29575688/README.md#newcode23 README.md:23: * [ninja](https://www.ninja-build.org) On 2017/10/16 15:27:12, sergei wrote: > It seems it would be good to specify minimal versions of both meson and ninja > because there are new features and bugfixes in newer versions which can be > essential for us. For meson, I can even add a required version. https://codereview.adblockplus.org/29527808/diff/29575688/README.md#newcode38 README.md:38: ninja On 2017/10/16 15:27:12, sergei wrote: > What about > ninja -C build? Done. https://codereview.adblockplus.org/29527808/diff/29575688/meson.build File meson.build (right): https://codereview.adblockplus.org/29527808/diff/29575688/meson.build#newcode107 meson.build:107: compiler_args = defines_args + generation_args + optional_args + ADDITIONAL_PARAMS On 2017/10/16 15:27:12, sergei wrote: > optional_args are included two times, the second time on the lines 118 and 166. good catch.
https://codereview.adblockplus.org/29527808/diff/29575688/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29575688/README.md#newcode23 README.md:23: * [ninja](https://www.ninja-build.org) On 2017/10/16 15:27:12, sergei wrote: > Just a notice, maybe add some tips about installing them, We generally don't do this, leaving that up to the vendor of the respective product - their preferred installation method can change. https://codereview.adblockplus.org/29527808/diff/29589640/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29589640/README.md#newcode34 README.md:34: old version of meson. Ubuntu 17.04 isn't LTS - it was the latest stable Ubuntu release until a few days ago.
https://codereview.adblockplus.org/29527808/diff/29589640/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29589640/README.md#newcode34 README.md:34: old version of meson. On 2017/10/26 19:27:00, Wladimir Palant wrote: > Ubuntu 17.04 isn't LTS - it was the latest stable Ubuntu release until a few > days ago. I'll remove that sentence. Or if you prefer the whole installation section.
https://codereview.adblockplus.org/29527808/diff/29589640/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29589640/README.md#newcode34 README.md:34: old version of meson. On 2017/10/26 19:36:04, hub wrote: > On 2017/10/26 19:27:00, Wladimir Palant wrote: > > Ubuntu 17.04 isn't LTS - it was the latest stable Ubuntu release until a few > > days ago. > > I'll remove that sentence. Or if you prefer the whole installation section. I definitely prefer removing the entire installation section, meson maintainers will do a better job keeping it up to date.
According to other code-reviews some caught up meson, so, IMO it is worth landing. However, before I would like to fix the version of meson, in particular I think we should have third_party/meson and our own script using it. https://codereview.adblockplus.org/29527808/diff/29617582/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29617582/README.md#newcode22 README.md:22: * [meson 0.40.0+](https://www.mesonbuild.com) Can we set the version to exactly 0.43.0 because it is working on Windows? The latest one is not working right now. Actually, I still prefer to rather have it in third_party/meson and "call meson module" from our python script. Additional info: https://github.com/mesonbuild/meson/releases/tag/0.43.0, commit ID 5626df46453e73b63827c4542aae36443fbd928b. https://codereview.adblockplus.org/29527808/diff/29617582/README.md#newcode23 README.md:23: * [ninja](https://www.ninja-build.org) Could you please add that ninja executable should be available, e.g. by having a path to the directory with it in the PATH environment variable, because meson searches for it while generating build-files? Maybe later, we could avoid pollution of PATH variable and even automatically fetch ninja as dependency.
updated patch https://codereview.adblockplus.org/29527808/diff/29617582/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29617582/README.md#newcode22 README.md:22: * [meson 0.40.0+](https://www.mesonbuild.com) On 2017/12/12 17:37:29, sergei wrote: > Can we set the version to exactly 0.43.0 because it is working on Windows? The > latest one is not working right now. > > Actually, I still prefer to rather have it in third_party/meson and "call meson > module" from our python script. > > Additional info: > https://github.com/mesonbuild/meson/releases/tag/0.43.0, commit ID > 5626df46453e73b63827c4542aae36443fbd928b. I pinned 0.43.0 in the current patch. I'm not convinced by using it as a python module ; it would introduce another NIH for the build system. https://codereview.adblockplus.org/29527808/diff/29617582/README.md#newcode23 README.md:23: * [ninja](https://www.ninja-build.org) On 2017/12/12 17:37:29, sergei wrote: > Could you please add that ninja executable should be available, e.g. by having a > path to the directory with it in the PATH environment variable, because meson > searches for it while generating build-files? This conflict with Wladimir saying we shouldn't have the installation instructions here.
https://codereview.adblockplus.org/29527808/diff/29617582/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29617582/README.md#newcode22 README.md:22: * [meson 0.40.0+](https://www.mesonbuild.com) On 2017/12/12 17:37:29, sergei wrote: > Can we set the version to exactly 0.43.0 because it is working on Windows? The > latest one is not working right now. Is there an issue on it already? If not, can you file one? Insisting on outdated versions or even replicating them in our repository generally isn't a good idea, we should rather make sure that the bugs are fixed. Normally, we can expect new meson releases to be backwards compatible, a meson release breaking things for us is rather unlikely - not a good justification for drastic measures. https://codereview.adblockplus.org/29527808/diff/29638561/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29638561/README.md#newcode21 README.md:21: * [Python 2.7 & 3](https://www.python.org) Python 3 is a transitive dependency for Meson, we shouldn't be mentioning it here explicitly. E.g. it isn't unthinkable that some future Meson distribution for Windows will install its own Python 3 copy - a user who follows our instructions here will have wasted time installing Python 3. https://codereview.adblockplus.org/29527808/diff/29638561/compile File compile (right): https://codereview.adblockplus.org/29527808/diff/29638561/compile#newcode23 compile:23: print "\tninja -C build" Is there a point in having this? It's not like we have thousands of people rely on the compile script and we need to switch them over to meson now. Most likely, this change only affects the three of us - and we already know. Just remove this script?
I think we can continue it in the issue tracker in order to not stop the progress here. I have created one https://issues.adblockplus.org/ticket/6200. https://codereview.adblockplus.org/29527808/diff/29617582/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29617582/README.md#newcode22 README.md:22: * [meson 0.40.0+](https://www.mesonbuild.com) On 2017/12/14 10:57:01, Wladimir Palant wrote: > On 2017/12/12 17:37:29, sergei wrote: > > Can we set the version to exactly 0.43.0 because it is working on Windows? The > > latest one is not working right now. > > Is there an issue on it already? If not, can you file one? Insisting on outdated > versions or even replicating them in our repository generally isn't a good idea, > we should rather make sure that the bugs are fixed. Normally, we can expect new > meson releases to be backwards compatible, a meson release breaking things for > us is rather unlikely - not a good justification for drastic measures. Done, https://github.com/mesonbuild/meson/issues/2784.
with comments from issue 6200 I just reverted to require meson 0.40.0 or later. https://codereview.adblockplus.org/29527808/diff/29638561/README.md File README.md (right): https://codereview.adblockplus.org/29527808/diff/29638561/README.md#newcode21 README.md:21: * [Python 2.7 & 3](https://www.python.org) On 2017/12/14 10:57:01, Wladimir Palant wrote: > Python 3 is a transitive dependency for Meson, we shouldn't be mentioning it > here explicitly. E.g. it isn't unthinkable that some future Meson distribution > for Windows will install its own Python 3 copy - a user who follows our > instructions here will have wasted time installing Python 3. Done. https://codereview.adblockplus.org/29527808/diff/29638561/compile File compile (right): https://codereview.adblockplus.org/29527808/diff/29638561/compile#newcode23 compile:23: print "\tninja -C build" On 2017/12/14 10:57:01, Wladimir Palant wrote: > Is there a point in having this? It's not like we have thousands of people rely > on the compile script and we need to switch them over to meson now. Most likely, > this change only affects the three of us - and we already know. Just remove this > script? habits. I'll just remove compile.
LGTM
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
