-
Notifications
You must be signed in to change notification settings - Fork 3
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
New build system #4
Conversation
@hanno-becker, @dop-amin could you try this out and have a look if you are overall happy with the structure? |
903916d
to
4831124
Compare
4214c60
to
61e0a76
Compare
I added some CI today. Another limitation I encountered is that we can't run |
This part is no resolved by passing the TARGET from the root Makefile containing the test name. |
02ce887
to
56ec599
Compare
8526688
to
70a9aa2
Compare
Upon @hanno-becker's request: I added an option for building standalone artifacts. This introduces some limitations for the build, though:
|
6e1acce
to
85f29a4
Compare
Apparently there is no way to directly get the exit codes for the Armv7-M/Armv8-M cores as it's not part of the the semihosting API. This is different for the A profile. After checking with Richard on this, the easiest way to get the rc is to attach a debugger and set a breakpoint at the exit function, then you can read out the rc. |
Agree |
before we had run-<PLATFORM>--<TEST> - the double-dash was needed because the platform names contain - and this makes splitting the platform and the target again from the target name too tedious. I changed it to run-<PLATFORM>_<TEST> now, but that means we can't have _ in both platform and test. There are some tests with _, but I think that's bad practice anyway, so I'll rename them.
I'm fairly happy with this now. Waiting for some feedback from @dop-amin now. |
This reverts commit 70a9aa2.
Han points out that MacOS users need to add this to their
It should look like this:
|
ab98e02
to
d3d51b1
Compare
(1) I have ported all the tests - unfortunately, 5 of them (chunk, montgomery, ntt-1024, saber, schoolbook) are failing. I commented them out in the root Makefile so that CI still works. Unfortunately, I do not have the bandwidth to debug these. I'm rather convinced that this is not due to the new build system, but unfortunately, I cannot check because none of these tests has ever been setup to work with qemu. @hanno-becker, do you want to debug those or shall we remove them? These are all pre-dating SLOTHY, so they may not be needed here. (2) I don't quite know what to do with (3) I'll check now if the elfs build by the current build system are actually running on my MPS3 (M55+M85). If that all works fine, I'd like to get this merged ASAP, so I can add Cortex-M4 and Cortex-M7 support, so we can rebase https://github.com/slothy-optimizer/pqmx/tree/armv7m. Then I'd add the same build system to https://github.com/slothy-optimizer/pqax. Then, I think I'll need a drink. |
|
I re-ran the tests (that are working on qemu) on the M55 and M85. On M55, all tests are passing. On M85, the intmulntt test fails - I think we have never tested that one. I also created issues #5 , #7, #8 as discussed. I updated the README too to reflect the recent changes. This is ready to be reviewed now and I hope we can merge it soon. |
Amazing, @mkannwischer. This is a huge simplification, thank you very much. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this locally on Apple M1, QEMU only (no MPS3). Worked out of the box.
This proposes a new and much simpler build system with some advantages:
I have not migrated all tests and platforms yet, as I wanted to ask for feedback first.
So far I have ported two platforms (m55-an547, m85-an555) and two tests (ntt_kyber, and ntt_dilithium).
Adding a test only requires creating a new folder in tests/ containing a .mk file and including it in the root Makefile, e.g.,
It consists of:
Including this in the core
will add
{build,run,clean}-{m55-an547,m85-an555}_ntt-dilithium
make targets meaning that autocompletion works fine and we can continue to call something like:Adding a platform is similarly simple by just creating a new folder in
envs/
containing a Makefile.This one will be called from the root Makefile with the corresponding
SOURCES
andASMS
variables set.The Makefile itself then just needs to implement the {clean,build,run} targets. For example this is the Makefile for the m55: