Add checksum verification for all *basic* modules #80
Conversation
|
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.
AFAIK, this is what different Linux distributions do, including ArchLinux, VoidLinux (possibly with a helper script).
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.
That's nice! I haven't looked your code thoroughly, but I did notice three things I think can be addressed beforehand:
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 For the changes themselves, I'll see if I can go through them soon. |
marcomontevechi1
force-pushed
the
main
branch
3 times, most recently
from
715f556
to
1725272
Compare
|
Greetings!
Done!
I did that in the titles for now. Will do better in the messages after I fix the next step:
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! |
|
Hey Marco!
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
force-pushed
the
main
branch
from
1725272
to
641ea36
Compare
As suggested by @gustavosr8 here: cnpem#80 (comment)
marcomontevechi1
force-pushed
the
main
branch
3 times, most recently
from
50e7ae8
to
dd820bc
Compare
|
Hi, @gustavosr8! I made the suggested changes, or at least as long as I can tell. Please feel free to review the current code. I tested both builds here and they seem to work. Please let me know if there is anything else that should be done. |
|
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.
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
force-pushed
the
main
branch
from
dd820bc
to
acc6ecc
Compare
|
I apparently did a wrong force push after the rebase to add modbus and erased everything 🫠
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. |
As suggested by @gustavosr8 here: cnpem#80 (comment)
This script will grow a bit, so I organize it before making it grow. Function content is identated. Functions check_arguments and download are used at the end of script.
Add help function to be called when arguments are not called appropriately. Add call to help function when script is called with no arguments or with '-h' option.
Manually downloaded and checked each sha256sum for each epics module that is downloaded with lnls-get-n-unpack. Added the expected sums to .env file and passed it as arguments to Dockerfile. The variables are not actually used in this commit. Next commit shall add the actual sha256sum checking.
lnls-get-n-unpack now uses all variables defined in .env file as
arguments:
- shacheck function checks the hashes.
- Download function now iterates through pairs of arguments to get url
and hash for each module.
- Help function describes this usage and can now exit with error code
as argument.
install_epics, install_opcua, install_area_detector, install_modules and
install_motor:
- pass variables defined in .env as arguments to lnls-get-n-unpack and
install_from_github.
install_functions:
- Add sha as argument to install_from_github and
download_from_github. install_from_github passes sha as argument
to download_from_github.
marcomontevechi1
force-pushed
the
main
branch
from
75cc143
to
c513928
Compare
|
Managed to recover the commits after rebasing to add latest changes. |
|
Hey poor mortal! |
An initial maybe-naive attempt to solve #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.