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

Avoid building tests by default (ninja only) #5728

Closed

Conversation

NNemec
Copy link
Contributor

@NNemec NNemec commented Jul 27, 2019

For one of my projects, building the tests of a third party library causes tricky problems and is not worth the trouble. The easiest way to avoid this is to follow up a comment in the mesonbuild sources:

"XXX: Sometime in the future these should be built only before running tests."

This change achieves this for the ninja backend in a quite straightforward way, since most of the mechanism was already in place.

I did try hard to get it working for the Visual Studio / msbuild backend but ultimately gave up. All attempts to realize a separate meta-project for building the tests separately had side effects that broke CI and required a ever growing cascade of fixes in other places of the code. So ultimately I added a line to exactly preserve the behavior for Visual Studio.

@NNemec
Copy link
Contributor Author

NNemec commented Jul 27, 2019

I just had another look at the section "Contributing to Meson"/"Tests":
"All project tests follow the same pattern: they are compiled, tests are run and finally install is run."

This makes it impossible to write an effective test for this change - I would need to create a project test but only build default, don't run tests and verify that the tests are not built. (e.g. by inserting a compile error into the test code). Finally, run the default target to verify that this has been built successfully.

A regular project test or unit test can't do this. Are there any ways to customize the way how tests are performed and evaluated?

@xclaesse
Copy link
Member

You can add a project in test cases/unit, and add code in run_unittests.py that build it then assert the test binary is not in the build directory, then run the test to see that it builds tests.

@scivision
Copy link
Member

scivision commented Jul 28, 2019

This is generally a good enhancement idea. Currently I use if statements to skip test executable building, but it would be better to just be the default behavior.

@QuLogic
Copy link
Member

QuLogic commented Jul 28, 2019

Is build_by_default:false not sufficient here?

@NNemec
Copy link
Contributor Author

NNemec commented Jul 28, 2019

build_by_default acts on targets, not on tests. can be seen in the code I changed

@NNemec
Copy link
Contributor Author

NNemec commented Jul 29, 2019

I just noticed the test failure of "192 test depends" which is quite obviously directly caused by my changes. Digging in for a few minutes did not lead anywhere.
Similarly, I had a look at run_unittests.py and found that this would also take me significantly deeper than I had intended to dig at this point of time.
If anyone with a deeper understanding of mesonbuild wants to have a look at this issue, I would be grateful for your support. Otherwise, I will have to put this aside for the time being and see when I can invest some serious time into it.

@jpakkane
Copy link
Member

Would this really help in your case? If you need to run tests on your own code, it would build the subproject tests as well. It would only work if you build and install without running the tests at all as far as I can tell.

@NNemec
Copy link
Contributor Author

NNemec commented Jul 30, 2019

Exactly that's the goal: build and install a package without building any tests on it.

@NNemec
Copy link
Contributor Author

NNemec commented Jul 30, 2019

To clarify: the master project is not meson-based. It only calls meson to build one of its dependencies.

@NNemec
Copy link
Contributor Author

NNemec commented Aug 3, 2019

After two hours struggling to understand the unit test failures, I am still at a loss:
Running run_project_tests.py locally, I get the same two failures for common/133 and common/192 as in the CI build. Both failures disappear when I undo my changes, both are related to my changes, so clearly my changes caused these failures in some mysterious way. All clear so far.

However: When I try to run these project tests individually via meson.py & ninja they work just fine. The generated ninja.build file also contains the dependencies that I would expect after my changes, so to all I see, everything should work fine

Unfortunately, all my attempt to debug into the issues have so far led nowhere. Could someone please help me out on this?

@NNemec
Copy link
Contributor Author

NNemec commented Aug 10, 2019

After another long debugging session I could identify the problem:

The documented method for running tests is by calling 'ninja -C builddir test' -- in this case, ninja will first build the test dependencies (collected in the phony target 'tests') and afterwards call back into 'meson test' to actually perform the tests. In this case, my changes work just fine.

In run_project_tests.py, however, the routine '_run_test' first calls 'compile_commands' which performs a default build (e.g. 'ninja' without specifying an argument). Afterwards, it calls the function 'run_tests_inprocess' which directly performs the tests, without ever building the test dependencies.

I believe, the correct fix would be to perform a 'ninja tests' before calling 'run_tests_inprocess'. This will require some more work and investigation, since it needs to be implemented in a generic way for all backends.

@NNemec
Copy link
Contributor Author

NNemec commented Aug 10, 2019

Indeed, this will require significant effort. On Ninja, it would be fairly straightforward, since the necessary dependencies already exist in ninja.build - the only thing needed is to add one more item to the output of get_backend_commands in run_tests.py (build_tests_cmd), wire it through to _run_tests and use it there and one more step before calling run_tests_inprocess.

The difficulty, however, is to do the same for VS and XCode. As it seems, the files there do not have any concept of grouping all dependencies needed for RUN_TESTS. That means that my changes here do not merely break the CI build, but are fundamentally incomplete on VS and XCode.

Back to the drawing board...

@NNemec NNemec force-pushed the avoid-building-tests-by-default branch from 1be7335 to dbad368 Compare August 11, 2019 19:25
@NNemec NNemec force-pushed the avoid-building-tests-by-default branch from ba8ab13 to 21db188 Compare August 15, 2019 16:55
run_tests.py Outdated Show resolved Hide resolved
run_tests.py Outdated Show resolved Hide resolved
@NNemec NNemec force-pushed the avoid-building-tests-by-default branch from ef2f561 to 66d2a33 Compare August 17, 2019 10:29
@NNemec NNemec changed the title Avoid building tests by default Avoid building tests by default (ninja only) Aug 17, 2019
@NNemec NNemec force-pushed the avoid-building-tests-by-default branch from e0df44f to 05d0578 Compare August 17, 2019 13:37
@NNemec
Copy link
Contributor Author

NNemec commented Aug 18, 2019

@jpakkane - could you please have another look at this patch now? I have spent many frustrating hours struggling with msbuild, finally giving up and making this a ninja-only feature. The remaining CI failures are due to #5807 independent of this issue. Not sure how essential you consider having a new unit test to cover this new functionality? As discussed above, it appears to be non-trivial. All in all, I think this change is valuable as it is now and it would allow me to finally move on with my original project.

@NNemec NNemec force-pushed the avoid-building-tests-by-default branch from 05d0578 to b5ff2e0 Compare September 1, 2019 10:45
@dcbaker
Copy link
Member

dcbaker commented Sep 3, 2019

I see a couple of differences between this and my #5867 that need to be addressed before we can merge this:

@@ -823,7 +823,7 @@ def generate_tests(self):
cmd += ['--no-stdsplit']
if self.environment.coredata.get_builtin_option('errorlogs'):
cmd += ['--print-errorlogs']
elem = NinjaBuildElement(self.all_outputs, 'meson-test', 'CUSTOM_COMMAND', ['all', 'PHONY'])
elem = NinjaBuildElement(self.all_outputs, 'meson-test', 'CUSTOM_COMMAND', ['all', 'tests', 'PHONY'])
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should depend on all, that's likely to hide bugs in your meson.build or trigger unnecessary rebuilds when testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well possible - I never thought about trying to drop the 'all' dependency.

@NNemec
Copy link
Contributor Author

NNemec commented Sep 3, 2019

Hi @dcbaker - thanks for picking up this PR! I already spent much more time on this then I had ever intended. By now I moved on to a completely different project and have no idea when I might get back to this. I feel no special attachment to the code, so feel free to continue my work in whichever direction you find appropriate. Ultimately, I'd be happy just to know that the hours I have put into this were worth something to someone...

@dcbaker
Copy link
Member

dcbaker commented Jan 25, 2020

Supersceeded by #6511

@dcbaker dcbaker closed this Jan 25, 2020
eli-schwartz added a commit to eli-schwartz/meson that referenced this pull request Jan 8, 2025
When running `ninja all` we shouldn't build testsuite programs as these
might not be wanted e.g. in order to just install the project. We do
want them to be built when running `ninja test`. Since meson 0.63 we
actually have a dedicated ninja alias for test dependencies -- move
these from the "all" rule to the dedicated test/benchmark rules.

Closes: mesonbuild#1704
Closes: mesonbuild#1949
Closes: mesonbuild#2518
Closes: mesonbuild#3662
Closes: mesonbuild#5728
Closes: mesonbuild#5867
Closes: mesonbuild#6511
Closes: mesonbuild#11317
Closes: mesonbuild#13378
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.

7 participants