CFlags being overriden instead of concatenated

[sidhujag]

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

set(CFLAGS "-O3 -funroll-loops -fomit-frame-pointer" CACHE STRING "")

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#L230

[AmineKhaldi]

Hi jagdeep,

Please note that CFLAGS in this context is not the environment variable that controls the C compiler flags, it's just a CMake variable that happens, in this particular case, to control an option in the Relic library's build.
You can see that they build up on this option and eventually concatenate the CMake variable that controls C compiler flags: CMAKE_C_FLAGS just some lines below the line you kindly highlighted (line 297).

I hope this helps!

[sidhujag]

@AmineKhaldi
Unfortunately its not a controlled variable, its part of CMake

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.

[AmineKhaldi]

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.

[wjblanke]

We can use a Chia fork (the same as the PR) of Relic until Relic upstreams it

[sidhujag]

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.

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?

[AmineKhaldi]

@sidhujag
Could you please test 667afa6 with COMP_FLAGS? Thanks.

[sidhujag]

@sidhujag
Could you please test 667afa6 with COMP_FLAGS? Thanks.

what specifically is needing to be set by your library that isn't set by relic upstream anyway? Why do you need to set these flags because most if not all are set upstream?

[dfaranha]

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.

[github-actions]

'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.'

[github-actions]

'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.'

[github-actions]

'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.'

[github-actions]

'This issue was automatically closed because it has been flagged as stale and subsequently passed 7 days with no further activity.'