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

Makefile keep track of variables #79

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

potsrevennil
Copy link
Contributor

@potsrevennil potsrevennil commented Jun 26, 2024

This PR resolves the issue mentioned in https://github.com/pq-code-package/mlkem-c-aarch64/issues/75

Signed-off-by: Thing-han, Lim <[email protected]>
@potsrevennil potsrevennil linked an issue Jun 26, 2024 that may be closed by this pull request
@potsrevennil potsrevennil added the benchmark this PR should be benchmarked in CI label Jun 26, 2024
@potsrevennil potsrevennil marked this pull request as ready for review June 26, 2024 05:22
@potsrevennil potsrevennil requested a review from a team June 26, 2024 05:22
Copy link
Contributor

@mkannwischer mkannwischer left a comment

Choose a reason for hiding this comment

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

Yes, @potsrevennil.

I verified that now the Makefile triggers a re-build when a different CYCLES flag is passed.

Maybe also keep track of the CFLAGS that are potentially passed from the outside?

@potsrevennil
Copy link
Contributor Author

potsrevennil commented Jun 26, 2024

@mkannwischer We cannot keep track of CFLAGS for now, since we chose to modify CFLAGS with environment variables (see discussion in https://github.com/pq-code-package/mlkem-c-aarch64/pull/69), and the current approach can only keep track of makefile variables.

Since the purpose of keep tracking of CFLAGS is that we want to avoid make clean while compiling for different architectures. There might be two ways of doing this:

  1. Add a makefile variable like EXTRACFLAGS, and set -mcpu=... via this EXTRACFLAGS, and keep track of it.
  2. Add a makefile variable like PLATFORM, and set mcpu, march flags accordingly, like mlkem-c-embedded have done

Maybe we can discuss this further in https://github.com/pq-code-package/mlkem-c-aarch64/issues/75, and open up a separate PR for solving this.

Copy link
Contributor

@mkannwischer mkannwischer left a comment

Choose a reason for hiding this comment

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

Okay, understood.
Let's merge this one first, and think about the CFLAGS separately.
For now users using custom CFLAGS need to run make clean in between, but I guess that is fine for now.

@mkannwischer mkannwischer merged commit ac842f8 into pq-code-package:main Jun 26, 2024
7 checks passed
@potsrevennil potsrevennil deleted the make-config branch June 26, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark this PR should be benchmarked in CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Makefile should properly keep track of variables
2 participants