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: Make tests and examples optional #49

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

Conversation

ryonakano
Copy link
Contributor

This allows building apps a little faster for developers who just want to include live-chart as a submodule of their app.

@@ -1 +1,3 @@
option('tests', type: 'boolean', value: true, description: 'Whether to run tests')

Choose a reason for hiding this comment

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

I would suggest instead marking the testsuite-only program with build_by_default: false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eli-schwartz Could you tell me how to make the test optional using build_by_default: false? This branch now always compiles the test:

user@elementary-8:~/work/live-chart$ meson setup builddir --prefix=/usr
The Meson build system
Version: 1.3.2
Source dir: /home/user/work/live-chart
Build dir: /home/user/work/live-chart/builddir
Build type: native build
Project name: live-chart
Project version: 1.10.0
C compiler for the host machine: cc (gcc 13.3.0 "cc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0")
C linker for the host machine: cc ld.bfd 2.42
Vala compiler for the host machine: valac (valac 0.56.17)
Host machine cpu family: x86_64
Host machine cpu: x86_64
Library m found: YES
Found pkg-config: YES (/usr/bin/pkg-config) 1.8.1
Run-time dependency gtk4 found: YES 4.14.2
Run-time dependency gee-0.8 found: YES 0.20.6
Build targets in project: 12

live-chart 1.10.0

  User defined options
    prefix: /usr

Found ninja-1.11.1 at /usr/bin/ninja
user@elementary-8:~/work/live-chart$ meson compile -C builddir/
INFO: autodetecting backend as ninja
INFO: calculating backend command to run: /usr/bin/ninja -C /home/user/work/live-chart/builddir
ninja: Entering directory `/home/user/work/live-chart/builddir'
[118/118] Linking target tests/livechart-test
user@elementary-8:~/work/live-chart$ ls -la builddir/tests               
total 600
drwxrwxr-x 3 user user   4096  1月 22 12:22 .
drwxrwxr-x 9 user user   4096  1月 22 12:22 ..
-rwxrwxr-x 1 user user 599848  1月 22 12:22 livechart-test
drwxrwxr-x 3 user user   4096  1月 22 12:22 livechart-test.p
user@elementary-8:~/work/live-chart$ 

Choose a reason for hiding this comment

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

Ah sorry I should probably clarify this is based on https://mesonbuild.com/Release-notes-for-1-7-0.html#test-targets-no-longer-built-by-default

It is technically semantically correct for build_by_default to behave the way I suggested (and is implemented in the release notes), but meson has been buggy for a while and built them anyway. I recently got around to fixing that, and the current release candidate for meson 1.7.0rc2 will avoid compiling the test.

I am not sure it makes sense to introduce a new option that, effectively, only matters for people using something other than the latest version of meson (which will be the case within a week or two once the final release is out) and additionally is only a speed improvement. Better to be forward-looking here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, is it only meson >= 1.7.0rc2 that making tests optional using build_by_default: false works as expected, right? I think we should prefer the previous way (providing a new option) than this way then at least for now, because live-chart is a library, not an app, so we shouldn't force people who want use it to use really newer versions of dependencies and compilers.

@lcallarec
Copy link
Owner

LGTM

@ryonakano : there is still an open conversation. Tell me if I can merge your PR.

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.

4 participants