-
Notifications
You must be signed in to change notification settings - Fork 288
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
export DEFINES for sub makefile #600
base: main
Are you sure you want to change the base?
Conversation
Suggest moving The current PR exports all DEFINES (including ones added after the export line, as can be verified by defining one of these values and looking at the full build output), but only when OVERRIDE_SECURITY_POLICY is defined. This does not seem quite correct, the suggested change exports all DEFINES to all subsidiary builds always, which also works and seems more sane/consistent. |
@ mikebeaton Thanks for your reminder.
Yes, let's move |
@dennis-tseng99 I tested the export on its own line before the DEFINES and the build hangs, debugging with --debug=all shows a lot of |
@bryteise Sorry, I cannot reproduce your 'hangs' issue. In my site, build is always successful. For example: |
@dennis-tseng99 Hrm I'm using GNU make 4.4.1 and using the current master. If I add I can replicate the issue under an archlinux container with binutils, gcc, make and git installed (you don't need binutils or gcc but you'll see errors related to them missing repeating when you run |
I can confirm the error with Update: Current Fedora container on Docker (which is indeed make 4.4) does show the same issue. The location shown in @dennis-tseng99's image is not the first |
Just
(although unlike in 4.4 |
I've checked all the Several others (i.e. The upshot being, a possibly more correct fix would be to specifically process OVERRIDE_SECURITY_POLICY in |
@mikebeaton To consider the code expanding in the near future, I still suggest we should make use of "export DEFINES" in Make.defaults to pass down variable(s) to sub-make. Besides, although shim.h is already included in lib/*.c, when user press make CLI command like
@bryteise Yes, you are right. I can reproduce your hanging problem now. This is because export directive is only used for old GNU make, e.g. 4.2.1. Let's go back to |
Okay, that also sounds valid to me - thanks. I'll close my alternative suggestion. |
@bryteise Would you please re-submit your PR which is moving the |
Signed-off-by: William Douglas <william.douglas@intel.com>
@dennis-tseng99 Updated, I think that's what you are looking for. |
Fixes #596 for me.