search

relic-toolkit / relic

Code
Other
458 stars 179 forks source link

Rename CFLAGS to COMP_FLAGS #207

Open AmineKhaldi opened 3 years ago

AmineKhaldi commented 3 years ago

This is my first attempt to contribute so I'll use this as a chance to thank you for this project.

This PR is a reaction to https://github.com/Chia-Network/bls-signatures/issues/267 where the name CFLAGS used for the CMake logic is not adequate as it has special meaning for CMake itself as documented in https://cmake.org/cmake/help/latest/envvar/CFLAGS.html

I just picked a name that is closest to the existing one, although my favorite would be something like RELIC_CFLAGS as the prefixing would make CMake users of the library intuitively see that it's Relic-specific, so I'm open to ideas about naming it if the fix itself is reasonable.

dfaranha commented 3 years ago

Hi, I read the whole thread and do not understand why changes are needed on RELIC's side. If you don't set CFLAGS, RELIC will just pick something sensible and go with that.

AmineKhaldi commented 3 years ago

Thanks for looking into the thread.

The relevant bit is https://github.com/Chia-Network/bls-signatures/issues/267#issuecomment-894828052 where the main problem is that in order to pass a custom version of this value to relic from its CMake users, you're forced to set it, and that interferes with CMake's custom logic around this specific value name (as the CMake docs link shows).

That is completely unnecessary if we just pick a name that doesn't interfere the CMake builtin/custom logic.

sidhujag commented 3 years ago

Thanks for looking into the thread.

The relevant bit is Chia-Network/bls-signatures#267 (comment) where the main problem is that in order to pass a custom version of this value to relic from its CMake users, you're forced to set it, and that interferes with CMake's custom logic around this specific value name (as the CMake docs link shows).

That is completely unnecessary if we just pick a name that doesn't interfere the CMake builtin/custom logic.

It can also be fixed if you also just append the variables because Relic toolkit will just use the ENV you set otherwise will set it to something sensible. I also asked why chia needs to set these ENV variables when they are the same as relic anyway?

Solutions: 1) append CMAKE in chia, then relic will just use that as it is. 2) rename to a controlled variable and have relic update to that like it was doing previously 3) chia just removes the ENV variable completely and relies on relic setting which is pretty much identical to what chia is doing anyway

In order of preference, 3 then 1 and then 2 is my intuition.

dfaranha commented 3 years ago

I would go for 3 to minimize the changes. Last time I renamed COMP -> CFLAGS the resulting breakage in other projects was wide and far.

PS: I understand the point about avoiding collision with CMake variables, but I think the current logic at RELIC is not invasive.

sidhujag commented 3 years ago

I would go for 3 to minimize the changes. Last time I renamed COMP -> CFLAGS the resulting breakage in other projects was wide and far.

PS: I understand the point about avoiding collision with CMake variables, but I think the current logic at RELIC is not invasive.

I agree its not, you guys are appending anyway and setting if not exists.. its non-invasive