Skip to content
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

Introduce a makefile generator #86

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Oct 29, 2023

First step towards #85.

Introduce a Python script that generates a makefile.

The design is heavily based on mbedtls-prepare-build. Most of the code is copied from there, but I took the opportunity to clean it up.

This is incomplete and not yet usable for daily work. I don't want to put further time into it until the design and current code is approved.

Current features:

  • Supported targets: lib, clean, help, .
  • Has correct dependencies.
  • Supports in-tree and out-of-tree builds.
  • The makefile generation relies only on Python ≥3.5.
  • The makefile relies only on POSIX make.
  • Generates include/tf_psa_crypto/version.h. This part is very ugly because the template file's format relies heavily on CMake, and I think the best way format would be to change the way this template works. But that's a job for a follow-up.
  • You can use custom configuration files in an out-of-tree build, but it requires manual intervention.

Features to be added in follow-ups:

  • Support programs and tests.
  • Good support for a different configuration (*config.h) in the build tree.
  • Improve support for customizing CFLAGS and such.
  • Windows support.
  • Abbreviate commands in the make output.
  • libtestdriver1
  • Support for option presets (e.g. an easy way to request an Asan build)?
  • Transparently generate the makefile if using GNU make (maybe BSD make too?).

@gilles-peskine-arm gilles-peskine-arm added size-s Estimated task size: small (~2d) needs-review Every commit must be reviewed by at least two team members needs-reviewer This PR needs someone to pick it up for review labels Oct 29, 2023
In out-of-tree builds, populate the build tree with a source backlink
and with empty include directories allowing overriding headers (e.g.
configuration file that's specific to a particular build tree).

Signed-off-by: Gilles Peskine <[email protected]>
Compile .c files and create the static library with `make lib`.

Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Parse `#include` lines. Look for headers in the source tree.

Note that dependencies in the build tree are not supported yet.

Signed-off-by: Gilles Peskine <[email protected]>
Add support for generated include files.

Generate include/tf_psa_crypto/version.h from
include/tf_psa_crypto/version.h.in and CMakeLists.txt. This is pretty
ugly, because the current method for injecting that information relies
heavily on CMake.

Signed-off-by: Gilles Peskine <[email protected]>
Copy link
Contributor

@daverodgman daverodgman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is not the right direction to go in; we already have a makefile generator system, in CMake.

If we want to support Make (which isn't a given, but maybe we can't avoid it), I would prefer to ship a very simple hand-written Makefile. We can be conservative about dependencies if that's a concern, i.e. rebuild everything if a .h file changes, etc.

@daverodgman daverodgman added the needs-design-approval Needs design discussion / approval label Oct 30, 2023
@gilles-peskine-arm
Copy link
Contributor Author

gilles-peskine-arm commented Oct 30, 2023

we already have a makefile generator system, in CMake.

Yes, but it's not good at its job.

I would prefer to ship a very simple hand-written Makefile. We can be conservative about dependencies if that's a concern, i.e. rebuild everything if a .h file changes, etc.

That might be ok for users, but it's no good for maintainers. Part of the reason I wrote mbedtls-prepare-build is to save time for myself. Even as the sole user, it's been well worth the time.

@ronald-cron-arm
Copy link
Contributor

ronald-cron-arm commented Oct 31, 2023

I am not sure about what you mean by "First step towards #65."?
The test components referenced by issue 65 use specific C compiler flags but having played a bit with it it seems possible to do that with cmake using something like cmake -DCMAKE_C_FLAGS=String:"-Werror -Wall -Wextra -mpclmul -msse2 -maes" .. for example.

@gilles-peskine-arm
Copy link
Contributor Author

I am not sure about what you mean by "First step towards #65."?

Typo: I meant #85.

@ronald-cron-arm
Copy link
Contributor

I am not sure about what you mean by "First step towards #65."?

Typo: I meant #85.

Ah, I could have guessed that, thanks.

@gilles-peskine-arm
Copy link
Contributor Author

Another thing that doesn't work with cmake (both in mbedtls and in tf-psa-crypto):

$ rm programs/psa/psa_constant_names
$ make programs/psa/psa_constant_names
make: *** No rule to make target 'programs/psa/psa_constant_names'.  Stop.

Running make builds everything, but I have no idea how to build a specific program.

@ronald-cron-arm
Copy link
Contributor

Another thing that doesn't work with cmake (both in mbedtls and in tf-psa-crypto):

$ rm programs/psa/psa_constant_names
$ make programs/psa/psa_constant_names
make: *** No rule to make target 'programs/psa/psa_constant_names'.  Stop.

Running make builds everything, but I have no idea how to build a specific program.

In CMake files we define the psa_constant_names target, thus make psa_constant_names just builds the psa_const_names program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-design-approval Needs design discussion / approval needs-review Every commit must be reviewed by at least two team members needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants