Add checksum verification for all *basic* modules

[marcomontevechi1]

An initial maybe-naive attempt to solve https://github.com/cnpem/epics-in-docker/issues/47

Add sha256sum -c to lnls-get-n-unpack. This covers almost everything but doesn't check areaDetector and motor modules yet.

In this approach, hashes are put manually by the developer, which I initially thought was a good idea... However, it does feel a little bureaucratic now that I look at it. Is there a better way? Or maybe should create a script that gets the hashes, but is just an developer auxiliary and is not run at build time?

Tested it and it builds both base and base/musl. Changing the hashes does make it fail like intended.

[henriquesimoes]

Hi, Marco! Thanks for the interest in contributing!

Sorry for the delay on giving you any feedback on this. We're currently going through a accelerator commissioning after a two-month shutdown. In about two weeks, beamlines will be having beam for initial tests. In summary, things are a bit busy by now.

In this approach, hashes are put manually by the developer, which I initially thought was a good idea... However, it does feel a little bureaucratic now that I look at it. Is there a better way?

AFAIK, this is what different Linux distributions do, including ArchLinux, VoidLinux (possibly with a helper script).

Or maybe should create a script that gets the hashes, but is just an developer auxiliary and is not run at build time?

Debian goes that direction, I'd say. Source packages contain SHA1 and SHA256 checksum, but the developer does not need to get into the specifics on how to generate and fill them, since packaging tools will do the job. (It does require you to have the tarball locally to be able to do so, nevertheless).

I think getting this to be done by an automatic script should be easier than documenting the specifics on how to do so.

Tested it and it builds both base and base/musl. Changing the hashes does make it fail like intended.

That's nice!

I haven't looked your code thoroughly, but I did notice three things I think can be addressed beforehand:

• In this project, we use a linear Git history. This means we rebase patches rather than creating merge commits. Thus, drop 351574e and rebase your branch on top of your current main. I suggest taking a look at git-rebase(1) manpage to familiarize yourself with the command, if you haven't used it before.
• You should follow our commit message header (and body) pattern (that is not properly documented yet), but that can be easily deduced by git logging files you have touched. In this case, it is mostly a base image change, so all of them would have a base: prefix followed by a title starting with an imperative verb in lowercase and ending with a dot.
• Your patches should not introduce bugs fixes to your own proposed patches. They should be well separated, atomic, and should document all relevant tradeoffs and rationale you made to implement the way you did. If you prefer, when we review them in details, we can tell which ones to squash, reorder or rewrite the message if any.

These things might seem not important at first, but in the long run they make a huge difference, since the Git history will be full of meaningful rationale that can easily queried by git-blame(1) and related Git tooling, and ease decision making. This is close to what both Linux kernel and Git project follow themselves, and is a good practice to collaborative projects.

For the changes themselves, I'll see if I can go through them soon.

[marcomontevechi1]

Greetings!

Thus, drop https://github.com/cnpem/epics-in-docker/commit/351574eef507e410bb29585b15b907d068bf9385 and rebase your branch on top of your current main.

Done!

You should follow our commit message header (and body) pattern

I did that in the titles for now. Will do better in the messages after I fix the next step:

Your patches should not introduce bugs fixes to your own proposed patches. They should be well separated, atomic, and should document all relevant tradeoffs and rationale you made to implement the way you did. If you prefer, when we review them in details, we can tell which ones to squash, reorder or rewrite the message if any.

For now I simply squashed the most obvious patch which was a fix to an obvious broken command. However, the commits are definitely not atomic since I had initially decided to add a new lnls-get-n-unpack from scratch and replace the old one with the new after it was all ready. I had decided to do that because although it's a simple bash script, at the time I didn't feel I could turn the old script into the new one in an small-increment series of commits.

What I intend to do now is to reorganize the commits in such a way that they still create a new lnls-get-n-unpack from scratch, but replace it with the new one only after it is actually finished (what I should have done from the start, anyway).

Please tell me if that is acceptable or if you require the original script to be modified in small increments for the PR to be accepted.

Cheers!

[gustavosr8]

Hey Marco!

Please tell me if that is acceptable or if you require the original script to be modified in small increments for the PR to be accepted.

Talking with @henriquesimoes and @ericonr we agreed that it's better if the changes are made in the original script to preserve the commit history from previous patches.

[marcomontevechi1]

Hi, @gustavosr8!

I made the suggested changes, or at least as long as I can tell. Please feel free to review the current code. Unfortunately the last commit is kinda big, but it follows the logic of being the smallest incremental/logical step that can be done after adding the shacheck function without breaking the build. All changes added beyond those in lnls-get-n-unpack.sh are just passing the needed hashes as arguments to the correct functions/files anyway.

I tested both builds here and they seem to work.

Please let me know if there is anything else that should be done.

[gustavosr8]

Hey Marco!

I think that a rebase is missing. We added support for modbus in the last release, and the CI fails when building this module.

Unfortunately the last commit is kinda big, but it follows the logic of being the smallest incremental/logical step that can be done after adding the shacheck function without breaking the build.

About that, WDYT on making a previous commit just adding the module's SHA256 and mentioning that it will be used in the future, separating it from the commit where you actually implement the SHA check and use it in the download and install functions?

[marcomontevechi1]

I apparently did a wrong force push after the rebase to add modbus and erased everything 🫠 The arcane art of recovering lost commits like these is still unbeknownst to this poor mortal. Will start rewriting the stuff.

About that, WDYT on making a previous commit just adding the module's SHA256 and mentioning that it will be used in the future, separating it from the commit where you actually implement the SHA check and use it in the download and install functions?

Yup, seem fine to me. I'm a bit bothered by the thought that it won't be a "completely atomic" commit, but it won't break the build anyway.

[marcomontevechi1]

Managed to recover the commits after rebasing to add latest changes. Split the last commit into two, first adding the expected hashes and then adding the actual function checking. Tested the build here and it seems to work.

[gustavosr8]

Hey poor mortal! Now it looks good to me (and also for the ci) :) Lets just wait for a double check from @henriquesimoes or @ericonr before merging. Thanks for your patience so far!