Makefile: track environment variable changes #632
Conversation
|
Ahh! I've pushed an older version of the commit. Let me fix this now. |
| OPT ?= 1 | ||
| RETAINED_VARS := CROSS_PREFIX CYCLES OPT AUTO | ||
|
|
||
| ifeq ($(AUTO),1) |
There was a problem hiding this comment.
Note: We have a divergence here between mldsa-native and mlkem-native. In mlkem-native, auto.mk is unconditionally included from Makefile after config.mk. And within auto.mk, we check for AUTO.
As far as I see (but I'd prefer another check, @mkannwischer @L-series), things work as intended even with the include at this point, but we should align this between the repositories.
There was a problem hiding this comment.
-
I think this will be consolidated when we port Makefile: Add compile-time compiler+host platform feature detection mlkem-native#1146
-
There is an issue for that Port compile-time feature detection #601
There was a problem hiding this comment.
Would it make sense to unify the the two config.mk as of now in preparation for the addition of compiler/host feature detection? It doesn't seem to me like any functionalities would break. Please let me know if I am missing something.
There was a problem hiding this comment.
I think it's easier to port the entire PR.
Unless there is a problem in this PR, let's merge this one first and then do the port in a follow-up PR. Do you want to give that a try @L-series?
There was a problem hiding this comment.
If you would like to contribute more, we should add you as a contributor so you can do PRs from a local branches, so we don't have to use the hacky workaround #633.
(Context: Some part of the CI only works for PRs from local branches as it needs to authenticate to AWS EC2 to spin up machines).
L-series
force-pushed
the
track_env_vars
branch
3 times, most recently
from
2fac292 to
6dd90f5
Compare
Previously, only values variables passed as Makefile variables (make VAR=value) triggered rebuilds when changed. Now environment variables (VAR=value make) are also tracked. Signed-off-by: Andreas Hatziiliou <andreas.hatziiliou@savoirfairelinux.com>
L-series
force-pushed
the
track_env_vars
branch
from
6dd90f5 to
9956a6d
Compare
There was a problem hiding this comment.
Thanks @L-series. I tested that this works as expected and fixes the problem (I reproduced that on main). This is good to merge.
However, we can't merge from a fork. Otherwise, we trigger warnings like https://github.com/pq-code-package/mlkem-native/security/code-scanning/7. We'll instead merge #633.
3 participants
This PR adds the functionality in which buils will now also be retriggered by changes to environment vairables.