search

MesserLab / SLiM

SLiM is a genetically explicit forward simulation software package for population genetics and evolutionary biology. It is highly flexible, with a built-in scripting language, and has a cross-platform graphical modeling environment called SLiMgui.
https://messerlab.org/slim/
GNU General Public License v3.0
160 stars 30 forks source link

Add make test feat #449

Closed currocam closed 1 month ago

currocam commented 2 months ago

I'm no expert, but I think this would solve #438

I tested it by doing:

  1. Generate build system 
    cd SLiM
mkdir build
cd build
cmake ..
  2. Run test before building 
    make test
  3. Clean files, build and then test 
    make clean
make
make test
  4. Edit some of the test files and repeat 1-3 to assert the tests actually fail when they should
bhaller commented 2 months ago

@bryce-carson thoughts?

bhaller commented 2 months ago

Thanks for the contribution @currocam!

bryce-carson commented 2 months ago

@bhaller, I am not familiar with writing tests in CMake either, but this looks alright to me.

I'd like to see GitHub workflows updated to take advantage of the CMake tests, however, rather than calling the tests manually. Ideally, make test should be robust enough to detect any errors on all systems.

I'm not familiar with GitHub workflows either, however; I've played with them before, but it was a bit of a struggle for me then. I won't say that we should delay accepting the PR until we integrate make test into the workflows, but it's something we should open an issue for to track progress on at least.

currocam commented 2 months ago

Hi, I agree that it would be nice to use make test in the CI also, as it would be less "surprising" for people looking at the code from outside.

I hesitated to included in the PR because using slim -testSlim and slim -testEidos seems idiomatic enough to me and I'm not sure it would be worthy to add an abstraction just to avoid those two commands

bhaller commented 2 months ago

OK. I'll deal with this once I'm now on vacation. I think I know how to fix the CI to use it; agreed that that is a minor concern really, but it'd be nice forthe CI to test "make test" itself, so that's a compelling reason to make that change. I'll circle around to this within a month-ish -- also going to Evolution in Montreal soon, so life is a bit busy right now. Thanks folks!

bryce-carson commented 2 months ago

[I'm] going to Evolution in Montreal

I'm pretty sure Jon Mee will be there presenting the work I did with him and Sam Yeaman. If you see him there say hello and snag a picture!

bhaller commented 1 month ago

Hi folks. Back from my vacation. This PR makes sense to me and looks clean, thanks!

The problem is that at least some features of it require a later CMake version than we otherwise require. For example, support for FIXTURES_SETUP and FIXTURES_REQUIRED` was apparently (the CMake doc says, https://cmake.org/cmake/help/v3.30/prop_test/FIXTURES_SETUP.html) only added in CMake 3.7. For regular (non-SLiMgui) builds, we presently require just CMake 2.8.12. The goal here, I guess, is to enable a fix for #438. So, two questions.

One, @currocam, could this "make test" support be made conditional, executed only for CMake version 3.7 and later? That looks easy (https://stackoverflow.com/a/43697419/2752221), I can do it after merging this PR if you're busy. Note that VERSION_GREATER_EQUAL, which would be the natural thing to use, was itself added in 3.7 and thus cannot be used; it looks like doing an if with VERSION_LESS that does nothing, and then doing this test stuff in the else clause of that if (to get VERSION_GREATER_EQUAL behavior without using VERSION_GREATER_EQUAL) is necessary, sigh. (Or use VERSION_GREATER with 3.6.9 or some such; 3.6.3 was the last 3.6, unlikely there will ever even be a 3.6.4.)

Two, @bryce-carson, will the build system that you want to use this feature in, for #438, actually have a sufficiently recent CMake version present, in all cases? If not, then this whole approach will not work for your purposes and we need a different approach.

Given this, I might not change the CI workflow to use the new "make test" facility, since I think some of the older versions we test on don't have that recent of a CMake version. I'm not sure how to check that directly, but I guess I can simply try switching over, and see which CI builds break :->. But even if we only use this facility in Bryce's stuff for #438, and don't otherwise document/support it (because of the version requirement), that's fine with me; I don't feel that this needs to be user-visible, really. Nobody has ever asked for it. Thoughts?

bryce-carson commented 1 month ago

Hi @bhaller,

The feature would only be used in the RPM builds, actually.

Here's some references which show, in the first document, how and why to use cmake tests in a spec file, and the second is the CMake documentation which explans how the feature taken advantage of in the first document is used.

It really is just a "quality of life" thing for the RPM spec file so that it only reads as

%prep
%setup -q

%build
%cmake -DBUILD_SLIMGUI=ON
%cmake_build

%install
%cmake_install

%check
%ctest

Rather than

%prep
%setup -q

%build
%cmake -DBUILD_SLIMGUI=ON
%cmake_build

%install
%cmake_install

With the latter, present code, I don't even run the tests during the RPM build.

bryce-carson commented 1 month ago

It seems like there's too much overhead to worry about with this, so I'm unsure if we should bother. The CI builds are stable right now, and I can also call the tests manually in the RPM spec file with as many (or one more) new lines as the following adds.

%check
%ctest
bhaller commented 1 month ago

Hi @bryce-carson,

The feature would only be used in the RPM builds, actually. ... It really is just a "quality of life" thing for the RPM spec file... With the latter, present code, I don't even run the tests during the RPM build.

Right, I understand this, I think. Although I'd say it's more than just "quality of life"; #437 occurred because the tests weren't running during the RPM build, right? And that was a rather unpleasant bug to deal with since we didn't discover it until post-release. So the goal is to not have that happen again. :->

My question to you was: what version of CMake is involved in these RPM builds? Can we rely on it being recent enough (3.7 or later, seems like?) that @currocam's changes to the CMake machinery will run without version errors? If the answer to that is no, then we need a different approach than the one taken in this PR.

bhaller commented 1 month ago

It seems like there's too much overhead to worry about with this, so I'm unsure if we should bother. The CI builds are stable right now, and I can also call the tests manually in the RPM spec file with as many (or one more) new lines as the following adds.

%check
%ctest

Not sure I'm understanding this comment. A PR like this one is needed to make that work, right? Something needs to bind a "run tests" action to the actual execution of "slim -testEidos" and "slim -testSLiM". Not sure if we're having communication issues here or what. Should we zoom?

currocam commented 1 month ago

Hi!

Thank you for taking the time to look at this :). I'm having a hard time following the conversation also, but I've a few things I'd like to say:

  1. I didn't consider the Cmake version. The FIXTURES_SETUP feature is only used as a "tricky" way to make make test work even if you have not run make before. I think we can remove it without any issue. I think test are available in cmake v2.8.12
  2. If the %ctest macro executes the test after building the binary, then there is no need of the FIXTURES_SETUP to be used, as in the CI one can simply make sure to build the project before running the test (I think)
  3. I would gladly make a PR (separate or not) updating the CI, and we can check if it is worthy or not.
bhaller commented 1 month ago

Hi!

Thank you for taking the time to look at this :). I'm having a hard time following the conversation also, but I've a few things I'd like to say:

  1. I didn't consider the Cmake version. The FIXTURES_SETUP feature is only used as a "tricky" way to make make test work even if you have not run make before. I think we can remove it without any issue. I think test are available in cmake v2.8.12
  2. If the %ctest macro executes the test after building the binary, then there is no need of the FIXTURES_SETUP to be used, as in the CI one can simply make sure to build the project before running the test (I think)
  3. I would gladly make a PR (separate or not) updating the CI, and we can check if it is worthy or not.

Hi @currocam. Thanks! Yes, removing the fixture stuff seems great, if it was not needed. I just checked the cmake 2.8.12 doc, and it looks like the other stuff you're using was all present that far back, so we should be good to go. I will merge this now.

bhaller commented 1 month ago

And sure, @currocam, go ahead and make a PR that switches the CI over to using this; I appreciate the help. It ought to work for all targets, since it works with cmake 2.8.12, so there should be no problems with it at all. Thanks!

bryce-carson commented 1 month ago

what version of CMake is involved in these RPM builds? Can we rely on it being recent enough (3.7 or later, seems like?) that @currocam's changes to the CMake machinery will run without version errors?

For Arch Linux we can rely on this. Arch has, essentially, the newest software possible for almost every package they ship in their official repository, and most AUR packages are built from the latest upstream source. We can rely on it having a CMake as recent as Fedora Rawhide if not the development version of CMake. Arch does not use the RPM, to be clear.

For Fedora, Red Hat Enterprise, and openSUSE (which all use the RPMs from Copr) we can rely on that.

For Ubuntu we cannot, as our earliest supported version needed the patch I introduced to deal with CMake versions (specifically).

For Debian we cannot, because likewise our oldest supported version has even older package versions than Ubuntu (if I recall correctly).

bryce-carson commented 1 month ago

It seems like there's too much overhead to worry about with this, so I'm unsure if we should bother. The CI builds are stable right now, and I can also call the tests manually in the RPM spec file with as many (or one more) new lines as the following adds.

%check
%ctest

Not sure I'm understanding this comment. A PR like this one is needed to make that work, right? Something needs to bind a "run tests" action to the actual execution of "slim -testEidos" and "slim -testSLiM". Not sure if we're having communication issues here or what. Should we zoom?

That's an RPM macro which just calls ctest, the executable shipped with CMake to run tests registered with CMake in the CMakeLists.txt file.

I can edit the RPM spec to simply run slim -testSLiM without relying on this PR at all, so CMake versions won't be any issue.

Arch doesn't use RPM, so @grahamgower just needs to include the lines to test slim and eidos if they aren't already there.

This mostly benefits CI to catch issues if we add a Fedora build, an Arch build, and more Ubuntu and Debian builds.

For packaging, it doesn't really benefit us at the moment (I think).

bhaller commented 1 month ago

what version of CMake is involved in these RPM builds? Can we rely on it being recent enough (3.7 or later, seems like?) that @currocam's changes to the CMake machinery will run without version errors?

For Arch Linux we can rely on this. Arch has, essentially, the newest software possible for almost every package they ship in their official repository, and most AUR packages are built from the latest upstream source. We can rely on it having a CMake as recent as Fedora Rawhide if not the development version of CMake. Arch does not use the RPM, to be clear.

For Fedora, Red Hat Enterprise, and openSUSE (which all use the RPMs from Copr) we can rely on that.

For Ubuntu we cannot, as our earliest supported version needed the patch I introduced to deal with CMake versions (specifically).

For Debian we cannot, because likewise our oldest supported version has even older package versions than Ubuntu (if I recall correctly).

OK; sounds like the CMake approach is not a general solution then. :-<

bhaller commented 1 month ago

...

That's an RPM macro which just calls ctest, the executable shipped with CMake to run tests registered with CMake in the CMakeLists.txt file.

I can edit the RPM spec to simply run slim -testSLiM without relying on this PR at all, so CMake versions won't be any issue.

Aha, that sounds like a good idea. I guess we pursued this approach because you wrote on #438: "@bhaller, if you can write a new section in the Makefile to run the tests, so that make test can be called, that'd be useful for the packaging process to detect issues like these in the future (it's not needed for the patched RPM; that's already live). I tried to locate the binaries and call the tests myself, but I couldn't find them where I expected them to be. It's easier to hand the work off to CMake since it magically knows where the binaries are." But if we can call the tests directly without relying on CMake, that seems clearly better.

Arch doesn't use RPM, so @grahamgower just needs to include the lines to test slim and eidos if they aren't already there.

This mostly benefits CI to catch issues if we add a Fedora build, an Arch build, and more Ubuntu and Debian builds.

For packaging, it doesn't really benefit us at the moment (I think).

Huh. I guess I misunderstood this also; I thought the reason that #437 was not detected until we shipped was that the tests were not being run during the RPM build, and that #438 was about adding the tests to the RPM build to prevent such a thing from happening to us again. But that is not the case? Sorry, I'm really having trouble following this; I guess I totally misunderstood the point of all this, then. :-< Well, there is certainly no harm to having merged this PR, even if it now seems to be rather useless; at least it cleaned up the symbol names in the CMake file. But is there a way for us to actually avoid having #437 type bugs bite us post-release? To me, that is the actual goal here.

bryce-carson commented 1 month ago

I'll need to spend some time reviewing things more intimately next weekend, or during an weekday evening, but #437 initially occurred in an Arch Linux build that @alfroids was using. @jshelly then ran into the issue with a special build of Fedora. It might be that the new default in GCC which sets the fortify_source level to 3 is present in that container build of Fedora, probably based off of Fedora Rawhide (bleeding edge, nightly builds of Fedora's upstream, untested sources).

I don't have the issue in a stable release of Fedora, and I don't recall it occurring during the COPR build of Fedora Rawhide, but I'll check the logs and what I said in the conversation details when I review this issue more closely later.

bryce-carson commented 1 month ago

Actually, @bhaller, I looked at the comment @jshelly made about their container, and they were running Fedora 39. I don't know why they ran into the issue. Perhaps the RPM packages have been broken since Fedora 39 and that was the first report of it that we had! I hope not! The software works on my machine after building from COPR and installing locally, so I don't know what the issue was, other than the forty_source=3 issue and the tests not being run.

Indeed. The tests were not run in the RPM package, which I'm really wondering why now. I know I previously had tests run, but I must've removed it when I refactored the spec file.

I tried adding them back in, but there was some difficulty in finding the binaries. I'll try again next Saturday.

After reading through some of my comments in that thread, I now remember why I wanted make test. I couldn't, despite a couple hours interest in it, find the binaries that were built. I ran multiple test runs and did various searches online, but the binaries weren't where I expected them to be and therefore I couldn't run the tests manually.

Now that we have make test (we do now, right?) then I'll definitely update the packages so that the tests are run on every COPR build.

bhaller commented 1 month ago

Hi @bryce-carson. Sounds like maybe it was more narrow in its scope than we realized, or maybe just nobody reported it. Well, I wouldn't spend time tracing through that history and trying to figure that out; we fixed that issue and it's done with. I think the goal now can just be to make sure that tests run during RPM and other installer/package builds, to avoid unexpected failures downstream in future.

Yeah, finding the binaries was the motivation for make test, as I understood it; CMake knows where it puts things. And yes, we now have make test, and it is now used in our GitHub Actions CI and works correctly, thanks to @currocam (see PR #455 for the final steps). And it does not require a later version of CMake than we otherwise require; he cleaned up that issue. So it should be ready for you to use for COPR. Thanks!

bryce-carson commented 1 month ago

Hi @bryce-carson. Sounds like maybe it was more narrow in its scope than we realized, or maybe just nobody reported it. Well, I wouldn't spend time tracing through that history and trying to figure that out; we fixed that issue and it's done with. I think the goal now can just be to make sure that tests run during RPM and other installer/package builds, to avoid unexpected failures downstream in future.

Yeah, finding the binaries was the motivation for make test, as I understood it; CMake knows where it puts things. And yes, we now have make test, and it is now used in our GitHub Actions CI and works correctly, thanks to @currocam (see PR #455 for the final steps). And it does not require a later version of CMake than we otherwise require; he cleaned up that issue. So it should be ready for you to use for COPR. Thanks!

Great!