Resolving name clashes with OpenSSL and others #30

sruesch · 2016-04-16T15:21:05Z

As stated in issue #16, there are some name clashes between Relic and OpenSSL, especially regarding BN_BITS, BN_BYTES and rsa_st, which are resolved here.

tfar · 2016-05-03T16:21:40Z

From my experience a project name prefix is more common than adding the project name as a suffix to public symbols. Would it be much work to change this? Other than that the change looks okay. Will try to fix our travis later and test this PR.

dfaranha · 2016-05-03T16:25:06Z

I would prefer a prefix, but inserting RELIC_ before everything would require rethinking some symbols. It's simply too long and verbose.

tfar · 2016-05-03T16:29:55Z

@dfaranha It is verbose, but the only way to avoid symbol conflicts. Is this mainly a usability concern of yours about the RELIC_? nacl for example uses a crypto_ prefix which is even longer.

dfaranha · 2016-05-03T16:30:46Z

We could try RLC_ and shorten some other symbols to reduce impact. Configuration is still a problem, RLC_NONE makes no sense.

tfar · 2016-05-03T16:32:18Z

Fine with that as long as we document it in our README that this is the library prefix of RELIC. It might not be obvious.

conradoplg · 2016-05-03T17:57:10Z

IMHO verbosity is a sad requirement of coding in C. It seems to me that the readability of RELIC is better than 2 characters saved by RLC...
And it would be even better to prefix the configuration by their names, e.g. RELIC_ARCH_NONE

dfaranha · 2016-05-03T18:19:19Z

Thanks, Conrado! That makes more sense. This library needs a major refactoring any way to stay relevant, so this is the right time to make such decisions.

Should we also consider prefixing library functions or is the current situation good enough?

tfar · 2016-05-03T18:20:27Z

Well, consistency would be nice. So I'd suggest prefixing all symbols that are part of the public API. This does not mean it all has to happen at once.

dfaranha · 2016-05-03T18:49:10Z

I am asking because we already support prefixes fox functions with the LABEL config variable.

conradoplg · 2016-05-04T02:03:53Z

Yes, I think prefixing everything would be better (or the "public API", but the problem is that almost everything is public...). The problem with LABEL is that the un-prefixed symbols are still defined as macros and could still clash with other stuff.
Maybe if in this refactoring we add support for multiple curves in the same compilation, then we won't need LABEL anymore... :D

tfar · 2016-05-04T06:03:56Z

Maybe if in this refactoring we add support for multiple curves in the same compilation, then we won't need LABEL anymore... :D

This would be quite some work, right? Especially with potential pre-computation tables, etc.

dfaranha · 2016-05-05T02:23:17Z

Yes. It will become slightly easier after we remove the memory allocators and some other slow functions.

dschuermann · 2016-05-05T09:28:42Z

So what's the requirement to get this merged into the current git master, omitting the idea to refactor other things like supported curves?
Prefix all names with "RELIC_" ?

tfar · 2016-05-05T09:46:50Z

I think so yes. For starters it should be enough to prefix just the conflicting symbols with RELIC_ and then merge this PR.

sruesch · 2016-05-10T16:09:02Z

I have changed the suffix to a prefix, so now it's RELIC_ instead of _RELIC. Is it acceptable like this?

tfar · 2016-05-10T18:53:07Z

bench/bench_eb.c

@@ -153,13 +153,13 @@ static void util(void) {
 }

 static void arith(void) {
-	eb_t p, q, r, t[EB_TABLE_MAX];
+	eb_t p, q, r, t[EB_RELIC_TABLE_MAX];


Why was RELIC_ added in between instead of at the beginning, i.e. RELIC_EB_TABLE_MAX?

I wanted to have RELIC_ with the context of TABLE, but if it's better as a proper prefix I can change it again?

It would seem more consistent I think. Just less to think about, if all RELIC symbols start with RELIC_. RELIC_EB_TABLE_MAX looks sensible to me. With that adjustment and the squashing mentioned by @dschuermann , this PR should be ready to merge. :)

dschuermann · 2016-05-11T06:04:47Z

@sruesch Better squash all commits to have only one commit, see http://blog.ona.io/general/2016/02/02/squashing-with-git-interactive-rebase.html

sruesch · 2016-05-11T11:48:50Z

@dschuermann Thanks, I'll do that!

sruesch · 2016-05-15T15:41:18Z

I have now changed all conflicting symbols so that they have the prefix RELIC_ as suggested and squashed the commits to only one commit. Is it acceptable like this?

tfar · 2016-05-16T21:59:31Z

From the changes made it sounds good. However, I'm seeing errors trying to build the default configuration (i.e. mkdir build && cd build && cmake .. && make):

Scanning dependencies of target test_fbx
[ 88%] Building C object test/CMakeFiles/test_fbx.dir/test_fbx.c.o
[ 88%] Linking C executable ../bin/test_fbx
[ 88%] Built target test_fbx
Scanning dependencies of target test_pc
[ 88%] Building C object test/CMakeFiles/test_pc.dir/test_pc.c.o
/Users/tobias/Development/rep/relic-toolkit/test/test_pc.c:371:9: error: use of undeclared identifier 'EP_RELIC_TABLE_MAX'
        g1_t t[RELIC_G1_TABLE];
               ^
/Users/tobias/Development/rep/relic-toolkit/include/relic_pc.h:85:26: note: expanded from macro 'RELIC_G1_TABLE'
#define RELIC_G1_TABLE                  CAT(G1_UPPER, _RELIC_TABLE_MAX)
                                        ^
/Users/tobias/Development/rep/relic-toolkit/include/relic_util.h:129:21: note: expanded from macro 'CAT'
#define CAT(A, B)                       _CAT(A, B)
                                        ^
/Users/tobias/Development/rep/relic-toolkit/include/relic_util.h:130:22: note: expanded from macro '_CAT'
#define _CAT(A, B)                      A ## B
                                        ^
<scratch space>:105:1: note: expanded from here
EP_RELIC_TABLE_MAX
^
…and more

sruesch · 2016-05-17T09:26:45Z

@tfar Oh! Sorry, I'll take another look!

dfaranha · 2016-05-18T00:03:24Z

This should be simple to address using the CAT macros to build RELIC_EP_TABLE_MAX.

tfar · 2016-06-14T21:21:41Z

@sruesch Any news on this PR?

sruesch · 2016-07-01T13:16:38Z

@tfar Sorry for my late reponse! I'm currently working on another project, but as soon as I have some free time I'll fix the errors. I hope to get to this during this weekend.

sruesch · 2016-07-27T12:54:56Z

@tfar I'm terribly sorry that it took me so long to come back to this! I have, as proposed, used the CAT macros to build RELIC_EP_TABLE_MAX. The current changed version compiles for me, please check again if this works for you, too, or if there are any more errors that I didn't catch. :)

tfar · 2016-08-30T12:56:23Z

Looks good to me, other than test_mdfailing https://travis-ci.org/relic-toolkit/relic/jobs/147748126 . But this could be unrelated to your change and probably was broken before (we should fix our CI script to report this as error on GitHub).
A local run on OS X 10.11 shows also two tests failing:

The following tests FAILED:
      5 - test_fpx (Failed)
     15 - test_md (Failed)

From my side I'm okay with merging this and fixing the broken unit tests later. What do you think @dfaranha ?

tfar · 2016-09-27T20:16:17Z

Merged it. Will try to debug the unrelated failing unit tests this or next week.

@sruesch Thanks again for this PR.

tfar reviewed May 10, 2016
View reviewed changes

sruesch force-pushed the master branch from 496a9ae to 87a0a8b Compare May 15, 2016 15:27

Resolve name clashes by prefixing with RELIC_

200a7a7

sruesch force-pushed the master branch from 87a0a8b to 200a7a7 Compare July 27, 2016 12:50

tfar merged commit 58e0956 into relic-toolkit:master Sep 27, 2016

tfar mentioned this pull request Sep 27, 2016

Prefix configuration defines with RELIC_ or similar to avoid conflicts when used with other projects #24

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolving name clashes with OpenSSL and others #30

Resolving name clashes with OpenSSL and others #30

sruesch commented Apr 16, 2016

tfar commented May 3, 2016

dfaranha commented May 3, 2016 •

edited

Loading

tfar commented May 3, 2016

dfaranha commented May 3, 2016 •

edited

Loading

tfar commented May 3, 2016

conradoplg commented May 3, 2016

dfaranha commented May 3, 2016

tfar commented May 3, 2016 •

edited

Loading

dfaranha commented May 3, 2016 •

edited

Loading

conradoplg commented May 4, 2016

tfar commented May 4, 2016

dfaranha commented May 5, 2016

dschuermann commented May 5, 2016

tfar commented May 5, 2016

sruesch commented May 10, 2016

tfar May 10, 2016 •

edited

Loading

sruesch May 11, 2016

tfar May 11, 2016

dschuermann commented May 11, 2016

sruesch commented May 11, 2016

sruesch commented May 15, 2016

tfar commented May 16, 2016

sruesch commented May 17, 2016

dfaranha commented May 18, 2016

tfar commented Jun 14, 2016

sruesch commented Jul 1, 2016

sruesch commented Jul 27, 2016

tfar commented Aug 30, 2016

tfar commented Sep 27, 2016

Resolving name clashes with OpenSSL and others #30

Resolving name clashes with OpenSSL and others #30

Conversation

sruesch commented Apr 16, 2016

tfar commented May 3, 2016

dfaranha commented May 3, 2016 • edited Loading

tfar commented May 3, 2016

dfaranha commented May 3, 2016 • edited Loading

tfar commented May 3, 2016

conradoplg commented May 3, 2016

dfaranha commented May 3, 2016

tfar commented May 3, 2016 • edited Loading

dfaranha commented May 3, 2016 • edited Loading

conradoplg commented May 4, 2016

tfar commented May 4, 2016

dfaranha commented May 5, 2016

dschuermann commented May 5, 2016

tfar commented May 5, 2016

sruesch commented May 10, 2016

tfar May 10, 2016 • edited Loading

Choose a reason for hiding this comment

sruesch May 11, 2016

Choose a reason for hiding this comment

tfar May 11, 2016

Choose a reason for hiding this comment

dschuermann commented May 11, 2016

sruesch commented May 11, 2016

sruesch commented May 15, 2016

tfar commented May 16, 2016

sruesch commented May 17, 2016

dfaranha commented May 18, 2016

tfar commented Jun 14, 2016

sruesch commented Jul 1, 2016

sruesch commented Jul 27, 2016

tfar commented Aug 30, 2016

tfar commented Sep 27, 2016

dfaranha commented May 3, 2016 •

edited

Loading

dfaranha commented May 3, 2016 •

edited

Loading

tfar commented May 3, 2016 •

edited

Loading

dfaranha commented May 3, 2016 •

edited

Loading

tfar May 10, 2016 •

edited

Loading