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

CFlags being overriden instead of concatenated #267

Closed
sidhujag opened this issue Aug 6, 2021 · 12 comments
Closed

CFlags being overriden instead of concatenated #267

sidhujag opened this issue Aug 6, 2021 · 12 comments

Comments

@sidhujag
Copy link

sidhujag commented Aug 6, 2021

Hi Guys,

It seems you guys are overriding CFLAGS which means any other flags passed in will be removed:

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
Copy link
Contributor

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 sidhujag closed this as completed Aug 7, 2021
@sidhujag sidhujag reopened this Aug 8, 2021
@sidhujag
Copy link
Author

sidhujag commented Aug 8, 2021

@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
Copy link
Contributor

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
Copy link
Contributor

wjblanke commented Aug 8, 2021

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

@sidhujag
Copy link
Author

sidhujag commented Aug 8, 2021

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
Copy link
Contributor

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

@sidhujag
Copy link
Author

@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
Copy link
Contributor

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
Copy link

github-actions bot commented Sep 2, 2021

'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
Copy link

'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
Copy link

github-actions bot commented Oct 2, 2021

'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
Copy link

github-actions bot commented Oct 9, 2021

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

@github-actions github-actions bot closed this as completed Oct 9, 2021
UdjinM6 pushed a commit to UdjinM6/bls-signatures that referenced this issue Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants