source code checksum doesn't work reliably

added Bug build system + 1 deleted label

changed milestone to %2021.4

As said before here, if there is consensus we can remove this again. I have no strong opinions about this any more.

mentioned in commit 8d0680fb

mentioned in merge request !1920 (merged)

This check is still useful to detect if a binary is built from modified code. And Plumed has never been receptive of our question for tagging GROMACS. Has that changed?

But I agree this functionality is fragile and we should consider removing it.

someone else then needs to come up with an alternative. !1920 (merged) takes care of removing all of the checksumming parts

@berk Did someone even ask? All it needs is for https://github.com/plumed/plumed2/tree/master/patches/gromacs-2021.diff to contain cmake/gmxVersionInfo.cmake in both the original pre-plumed form and one where GMX_VERSION_STRING_OF_FORK has a default value of PLUMED-x.y.

Yes, we asked the Plumed developers more than once some years ago. But I don't think anyone asked recently.

https://github.com/plumed/plumed2/issues/737

Thanks!

Further observations:

EasyBuild applies several trivial patches to GROMACS, mostly to make tests pass. That results in the build being reported as modified. DebiChem has had such patches also, not sure if currently true. bioconda has also had such patches e.g. to let MacOS+OpenCL work when we hadn't fixed a bug yet.

Spack also applies patches e.g. to let old GROMACS code with hard-coded CUDA SM versions build with new CUDA. That probably has the same result.

So there are probably quite a few users whose GROMACS log files announce that they've not been verified, when the reason is entirely unalarming. Those log files dilute the hoped-for value to GROMACS developers answering forum posts knowing whether the code is really modified.

I assume nobody thinks they can come up with and maintain a whitelist of files that are not OK to patch.

There are at least two major use-cases where I think the version tagging is warranted:

a source or binary (fork) is distributed through non-official channels with intentional changes to the source code;
"internal" / custom versions of GROMACS code are made by users or acquired through colleagues etc. from which a binary is compiled which is potentially reused by multiple users.

If either of the above is made knowingly and with intent, it is reasonable to expect that GMX_VERSION_STRING_OF_FORK is also set in the process and consequently if it is not set it is reasonable to inform users. The issue of well-known distributions of GROMACS not setting the suffix is I think an entirely separate one and in my view is better solved with improved documentation or downstream contribution as Mark did rather than removing the feature.

The third case of not being able to perform the checks can mask either of the above, and therefore it should be distinguished, otherwise the check is not reliable and creates a false sense of security.

I do not see an issue with the "-UNCHECKED" suffix, after all if we care about the integrity of a GROMACS installation, we want to communicate when we can confirm this (and we even have an explainer text next to it). If this is still undesired, we can turn the "UNCHECKED" suffix to something obscure to most users, e.g. "2021.3*" or something similar.

I agree there are advantages, and that some of the disadvantages are better addressed downstream.

it is reasonable to expect that GMX_VERSION_STRING_OF_FORK is also set

Only if they know it exists. There's no code that hints that when a modification is detected this might be useful. If we did have such a hint, it would be noisy in whatever cases to which we haven't downstreamed support for GMX_VERSION_STRING_OF_FORK, which is awkward.

https://gromacs.bioexcel.eu/search?q=unchecked demonstrates that UNCHECKED versions definitely happen, but infrequently.

Per my guess in the issue description, a user was concerned about UNCHECKED https://gromacs.bioexcel.eu/t/2021-beta3-unchecked/1318 (and the configure-time warning that Paul expected did not appear, perhaps fixed in #3862 (closed)) and didn't know what to do about it. There's probably more such people who never made a post. We should remember to weigh those disadvantages against the advantages.

CMake 3.16 file supports both a recursive glob that excludes directories (albeit case sensitive in different ways on different OS), and hashing a file, see https://cmake.org/cmake/help/v3.16/command/file.html. If the search order is stable across OS, then that could probably replace the python and eliminated the "unchecked" category. But I don't think investigating that is a good use of anybody's time until much later this year.

Instead of "not verified" you mean "modified", right? "not verified" was the original reason for starting this issue.

The note itself is purposely phrased in an unalarming fashion.

I think this is still useful as is. But I also agree it is not worth spending much developer effort on.

Yes, "not verified" and "modified" are caused by different situations, but the theme is similar.

On the same cluster today once the admins did an EasyBuild install for me, I see in the log file

GROMACS version:    2021.3-MODIFIED
This program has been built from source code that has been altered and does not match the code released as part of the official GROMACS version 2021.3-MODIFIED. If you did not intend to use an altered GROMACS version, make sure to download an intact source distribution and compile that before proceeding.
If you have modified the source code, you are strongly encouraged to set your custom version suffix (using -DGMX_VERSION_STRING_OF_FORK) which will can help later with scientific reproducibility but also when reporting bugs.

As a user, my intention is to do science by loading a module. If I follow the advice here ("make sure to download... and compile"), I'll either waste my time compiling my own (perhaps badly), or ask the sysadmins, who will likely tell me that all the software on the supercomputer is built using EasyBuild so the admins can benefit from the collective knowledge there, and no, they won't do a custom installation. So the message is pretty useless in relevant cases, whether users follow it or not.

As a GROMACS developer answering a forum post, if I see this I am none the wiser whether it is PLUMED or a custom fork or just a standard build provided by EasyBuild/Spack/yum/conda/apt/etc. To get that information, we need those projects to set GMX_VERSION_STRING_OF_FORK. We can do that whether or not we verify the code, but if we do then the gain case for verifying the code goes away.

Even though I have much better things to do, some clarification

I assume the problem is that the system python is python2 and so our code that checks the source code hash can't work. If so, it also won't work when python isn't found at all.

CMake tells you exactly that the reason for not verifying the checksum is that the Python you are using is not sufficient. This could have been a warning, but we didn't want to annoy people with it.

EasyBuild applies several trivial patches to GROMACS, mostly to make tests pass. That results in the build being reported as modified. DebiChem has had such patches also, not sure if currently true. bioconda has also had such patches e.g. to let MacOS+OpenCL work when we hadn't fixed a bug yet.

This is exactly the reason I wanted to have this kind of reporting. We can't be expected to spend our time adding things to whitelists (or worse manually verifying) everything someone else applies a patch to the source for whatever reason. If people want to repackage and ship a GROMACS that is not what we are distributing, people should know that, even if most of those changes are harmless. If they result in patches on our side this is even beneficial.

Spack also applies patches e.g. to let old GROMACS code with hard-coded CUDA SM versions build with new CUDA. That probably has the same result.

This kind of stuff is another reason why I think this should stay. We can't be expected to check if those patches have any effect other than what they are supposed to do. And people should stop using old GROMACS if possible (and we should make sure that there is no need for it -> hint at the table code)

I wont be spending any more time on this during my leave. !1920 (merged) is there if you want to get rid of this @mark.j.abraham

CMake tells you exactly that the reason for not verifying the checksum is that the Python you are using is not sufficient. This could have been a warning, but we didn't want to annoy people with it.

Indeed, but that sadly not available to the users of a cluster seeing their log files tell them the build is not verified.

This is exactly the reason I wanted to have this kind of reporting. We can't be expected to spend our time adding things to whitelists (or worse manually verifying) everything someone else applies a patch to the source for whatever reason. If people want to repackage and ship a GROMACS that is not what we are distributing, people should know that, even if most of those changes are harmless. If they result in patches on our side this is even beneficial.

A good way to do that is to have those packaging utilities use the suffixing so all will be able to find out what kind of modifications happened.

This kind of stuff is another reason why I think this should stay. We can't be expected to check if those patches have any effect other than what they are supposed to do.

Indeed, although that risk is pretty low for the aforementioned packaging projects, all of which are open source.

https://github.com/spack/spack/pull/25869 https://github.com/easybuilders/easybuild-easyconfigs/pull/13950

Thanks Mark for doing this.

We shouldn't be the ones having to do those modifications to other projects, but this is not a discussion to start here.

...

On Thu, 9 Sep 2021, 17:40 Mark Abraham (@mark.j.abraham), < gitlab@mg.gitlab.com> wrote:

Mark Abraham https://gitlab.com/mark.j.abraham commented on a discussion #4133 (comment 673741400):

https://github.com/spack/spack/pull/25869 https://github.com/easybuilders/easybuild-easyconfigs/pull/13950

— Reply to this email directly or view it on GitLab #4133 (comment 673741400). You're receiving this email because of your account on gitlab.com. If you'd like to receive fewer emails, you can unsubscribe https://gitlab.com/-/sent_notifications/REDACTED/unsubscribe from this thread or adjust your notification settings.

Well, the GROMACS developers are the ones who care about the outcome, so it's pretty much on us to guide those projects to use the tools we give them in order to help us.

Is this really something that's an urgent and important bug fix for the release-2021 series that we should prioritise higher than other work in the team?

Personally I don't think so (meaning: I think we should close it, or at least remove it as 2021.x milestones), but if people have time to spare and a concrete plan how it can be completed in a few days I'm open to be convinced :-)

Paul's !1920 (merged) is already up and "fixes" it in the sense that the build system will no longer depend on finding a working python3 in order to give users a smooth experience when nothing is wrong. When we discussed this a month ago in a Monday meeting, some people still liked the idea of the feature, but it seems that nobody has the time to put into a solid implementation.

I'm a believer in "Primum non nocere", so I just added my approval to !1920 (merged) as our short-term solution.

For what it's worth, assuming that this feature was aimed at promoting reproducible research (which I thought was the original goal), !1920 (merged) does the opposite of "primum non nocere".

mentioned in commit 092b9af3

Closed by !1920 (merged)

closed

Note, I'll still see that I get this working properly again once I'm back.

...

On Wed, 20 Oct 2021, 22:38 Mark Abraham (@mark.j.abraham), < gitlab@mg.gitlab.com> wrote:

Issue was closed by Mark Abraham

— Reply to this email directly or view it on GitLab #4133 (closed). You're receiving this email because of your account on gitlab.com. If you'd like to receive fewer emails, you can unsubscribe https://gitlab.com/-/sent_notifications/REDACTED/unsubscribe from this thread or adjust your notification settings.

As noted before, the role of checking is as far as I'm concerned not to force the hands of a few known fork maintainers and when these end up declaring the version string. That can be done through collaboration as Mark demonstrated. The main role of this feature is reproducible research, provenance, etc. Even if spack, easybuild, and plumed all start using proper versioning, there are / will be other forks and there will always be cases of user-modified sources (and modified tarballs will live on and be distributed even if not very widely), which we want to be able to detect, I think. I disagree that what is reported here is a major issue, it's never been reported as such by users (which should be our metric as opposed to opinions), but even if it is seen a such, reporting a checksum mismatch when there is python and this can be detected and reporting nothing when no checks can be done is better than just removing it (which has been proposed in meetings, but of course @acmnpv was probably not aware I assume).

On the other hand, if I am wrong and the primary role of the feature is no longer a priority, it would have been appropriate to make this as project-wide decision.

The only thing worse than making decisions that in hindsight were wrong is projects that aren't able to make decisions. As we've discussed above for a month, there are both advantages and drawbacks, and we eventually came to a decision about the least bad option for now.

In particular, we're not going to start go back and reopen issues or make things project-wide decisions, because then we're not even going to finish a dozen of all the open issues we have :-)

mentioned in commit 6765b60b

mentioned in merge request !3074 (merged)

mentioned in commit c539a534

mentioned in commit fb4df2b4

mentioned in commit dbdfb3fc

mentioned in merge request !3556 (merged)

mentioned in commit bd0ddb63

added 2021 label and removed 1 deleted label

source code checksum doesn't work reliably

Designs

Child items ...

Activity