-
Notifications
You must be signed in to change notification settings - Fork 213
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
CFlags being overriden instead of concatenated #267
Comments
Hi jagdeep, Please note that I hope this helps! |
@AmineKhaldi https://cmake.org/cmake/help/latest/envvar/CFLAGS.html It will be picked up from ENV and so its hard to pass in custom CFLAGS this assumed that CFLAGS will not be set by the build system. In our case CFLAGS is always set and so our custom flags get lost when you replace them. It works against the older version of your code that used the COMP field because COMP was a controlled variable, although relic does not use it either so it was a NOOP. Now I must reverse-engineer and pass in the fully qualified CFLAGS because I know that relic won't replace or append to it if it is set already. |
Hmm, the problem is, Relic are using this (see 2f451f7 on this side to accommodate for that) when IMHO they shouldn't. This names clash (and proper flags handling in general) could be fixed on Relic's side, eliminating the need to alter CFLAGS on this side at all, as CMake already initializes CMAKE_C_FLAGS from CFLAGS. I can send a PR to them suggesting a rename to COMP_FLAGS or any other name they see fit, if you find the above reasonable. |
We can use a Chia fork (the same as the PR) of Relic until Relic upstreams it |
Well you can also just append CFLAGS same as what relic is doing. Why do you need to set anyway? Relic seems to be setting the same ones (O2 not O3, O3 is not as safe and sometimes ends up slower is there a reason to use O3 optimization anyway)? Relic appends if ENV sets, otherwise just sets it, it so I don't see how its an issue upstream. I also noticed on the readme it says to use no-pie however you are not checking or enforcing this, is it required for ldflags? |
Hi, I don't understand why it should be changed at RELIC's side. The current handling is just to make it more convenient to change CFLAGS for different builds in comparison to changing CMAKE_C_FLAGS, something I do quite frequently when benchmarking. This should not affect any projects outside RELIC, but please enlighten me if I got this wrong. |
'This issue has been flagged as stale as there has been no activity on it in 14 days. If this issue is still affecting you and in need of review, please update it to keep it open.' |
'This issue has been flagged as stale as there has been no activity on it in 14 days. If this issue is still affecting you and in need of review, please update it to keep it open.' |
'This issue has been flagged as stale as there has been no activity on it in 14 days. If this issue is still affecting you and in need of review, please update it to keep it open.' |
'This issue was automatically closed because it has been flagged as stale and subsequently passed 7 days with no further activity.' |
Hi Guys,
It seems you guys are overriding CFLAGS which means any other flags passed in will be removed:
bls-signatures/CMakeLists.txt
Line 92 in ecf77cc
It should be
set(CFLAGS "${CFLAGS} -O3 -funroll-loops -fomit-frame-pointer" CACHE STRING "")
I guess. Or you can check out what Relic is doing here: https://github.com/relic-toolkit/relic/blob/main/CMakeLists.txt#L230The text was updated successfully, but these errors were encountered: