-
Notifications
You must be signed in to change notification settings - Fork 10
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
Downstream issues with reproducibility due to submodules #13
Comments
@tlaurion @kylerankin I leave to you the decision, implementation, and tests. |
@kylerankin @szszszsz : For Heads, I would recommend creating a new hidapi module (could be a dumb module just like coreboot-blobs is defined under libremkey-hotp-verification just like coreboot module does it), on which libremkey-hotp-verification would then depend, using the same git archive download trick, fixing extracting strip depth accordingly in module with a variation of how it's done for coreboot-blobs. Edit: point more directly to tricks currently used under Heads to fix current Purism/Nitrokey module reproducibility issues under Heads, permitting a change upstream in modules to download new git revision and build fresh. Otherwise, there is no reproducibility of builds, users/CI not redownloading sources, as proven in the past. |
@tlaurion I understand your proposition is a no-action for this project, but instead to configure it downstream. |
Remove __FILE__ macro used for the debug messages Fixes #13
Latest build links: |
Reopening, since the reproducibility problem still persist |
Hi @tlaurion ! Full strings diff
|
@MrChromebox @szszszsz @kylerankin @jans23 @osresearch: GitlabCI (ubuntu:18.04 docker image): df1ebcdbcc61c7dcd1ce3f8626e5389de47e9dc5143647d95492b1dab7b3b890 ./bin/libremkey_hotp_verification CircleCI(Fedora:30 docker image): Logs now available in downloadable artifacts: GitlabCI CircleCI Note that this patch against upstream code should be merged upstream instead of patched against it. Also note that librem-hotp-verification module is now based on commit d4948fd |
The initial change (setting the
This means the issue is occurring only during the Heads build, with that particular build-provided compiler configuration.
I will add in the further commit local build reproduction test with Docker and |
Object files artifacts from CI's (idea 1):
I plan to check this in the following days, unless someone would be faster. Edit: note to myself: track the |
I made a brief comparison and it appears the resulting code is not stable, as in this example: I have disabled all optimization flags and run rebuild on CIs. Waiting for the results. Will look into it in the following days. |
Attempt to turn off optimizations made it even worse. Now all files are with different checksums. Diffoscope reports, that symbols are not ordered. To check:
Builds:
Below difference example from |
Runs Docker images with Fedora 30 and Ubuntu 18.04, and reprotest in it. Issue #13
I am happy to report, that I got first SHA256 sums match between CIs while using Makefile as the builder, and perhaps solving the issue. The direct cause of the initial problem might be, that the CMake toolchain file for cross-compilation was missing some variable setting (either cross tool path, or flags) thus different end-result binaries. Other cause could be having different CMake binary and different flags set, as it seemed to not be rebuilt as opposing to Gnu Make, but taken from the OS repository. Hashes:
Commits for Heads giving the same result on both CIs:
Builds' links:
Edit: working hotp-verification commit: 809953b Additionally I had some problems on Gitlab CI, which were caused by invalid caching due to having the |
Allow to build the project with Makefile for the use of the reproducible Heads build. Reprotest small corrections. Details: #13 Fixes #14 Squashed commit of the following: commit 62963d8 Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Fri May 15 18:32:37 2020 +0200 Readme: describe new build ways commit 58db08d Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Fri May 15 17:39:52 2020 +0200 Make dependencies configurable with pkg-config and variables commit fafabdb Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Fri May 15 09:44:13 2020 +0200 Show Github archive sha256sum commit 809953b Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Fri May 15 09:40:59 2020 +0200 Correct out name commit 0b499fb Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Fri May 15 09:31:22 2020 +0200 Add install command commit 6a5c1d9 Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 18:51:01 2020 +0200 Meson: add cflags commit d5a141a Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 18:40:01 2020 +0200 Meson: add version.c generation commit 1b9b132 Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 18:33:14 2020 +0200 Add working meson build commit 85df1f9 Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 18:33:01 2020 +0200 Add missing include commit c51ccf7 Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 18:01:55 2020 +0200 Initial version for meson commit afe538c Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 18:01:34 2020 +0200 Add missing version.c.in handling commit 15f312d Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 17:45:48 2020 +0200 Refactor repro test commit bf89876 Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 17:45:32 2020 +0200 Remove Udev dependency commit 099c88b Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 17:08:21 2020 +0200 Clear remains of previous tests. Add BINARY var. commit cba28b0 Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 17:00:51 2020 +0200 WIP Builds with hidapi commit 3b0be18 Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 16:30:27 2020 +0200 WIP commit c7cf2ea Author: Szczepan Zalega <szczepan@nitrokey.com> Date: Thu May 14 16:14:14 2020 +0200 Initial version
@szszszsz : check the upstream circleci |
@szszszsz thanks a lot for this, just saw the update. Will look into details tomorrow, just started quick build during the night, changing simply the commit id to 06e5004 and retesting. https://app.circleci.com/pipelines/github/tlaurion/heads/165/workflows/cb0c014b-6806-42cc-ba17-596713a48c78/jobs/178 |
Actually you need to change the build command too, to use Makefile instead of the CMake. I have not found the direct cause in the latter yet, however I believe it might not be needed and Makefile build is sufficient for Heads as-is. The whole Heads is based on Makefiles anyway, and all dependencies are properly provided (including stable GNU Make rebuild during the process), which does not seem to be for CMake. Will use the CI patch, thank you for the heads up! |
don't hesitate to tag me back in, else not notified 😃 |
@tlaurion |
@szszszsz : please do against upstream, my branch is far from being up to date! Just change the modules required. I see some magic being applied to CIs that would be interesting to push as seperate PRs if you found some interesting magic there! If you do not care about ownership of those changes, I can play with them around, but would definitely prefer the maintainer of the mainstream project to maintain their modules inside of Heads. Will comment on my branch meanwhile. |
Runs Docker images with Fedora 30 and Ubuntu 18.04, and reprotest in it. Issue #13
Move from CMake build system to GNU Make for hotp-verification Change version to one supporting Makefile build Fixes linuxboot#724 Connected: - Nitrokey/nitrokey-hotp-verification#13 - linuxboot#722
@tlaurion Sure! Now that I've learned the Heads better I can maintain the project's package there. Just ping me and create a ticket of you need anything. About CI, I just added some defending code to remove old objects' cache (I've overlooked the cache path in the configuration) and added some debug artifacts, nothing fancy I think. If I could suggest anything, perhaps it is worth considering extracting build script to external file, instead of building it up there. That would allow to run it locally easier as well. |
@szszszsz sorry I do not understand. What would you add? |
Sorry, I meant extracting lines from the |
From: #12 (comment):
Example of used solution: https://github.com/osresearch/heads/blob/master/modules/musl-cross#L7
The text was updated successfully, but these errors were encountered: