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

Rename CFLAGS to COMP_FLAGS #207

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AmineKhaldi
Copy link

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 Chia-Network/bls-signatures#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
Copy link
Contributor

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

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.

@sidhujag
Copy link

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants