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

Fix Libs line in toxcore.pc pkg-config file. #325

Merged
merged 1 commit into from
Dec 14, 2016

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Dec 14, 2016

CMake lists are ; separated and CMAKE_THREAD_LIBS_INIT contains
"-lpthread". This resulted in "-l-lpthread;-lrt" on Linux.


This change is Reviewable

@iphydf iphydf added this to the v0.1.0 milestone Dec 14, 2016
CMake lists are `;` separated and CMAKE_THREAD_LIBS_INIT contains
"-lpthread". This resulted in "-l-lpthread;-lrt" on Linux.
@GrayHatter
Copy link

:lgtm_strong:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 14, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


CMakeLists.txt, line 528 at r1 (raw file):

################################################################################

string(REPLACE ";" " " toxcore_PKGCONFIG_LIBS "${toxcore_PKGCONFIG_LIBS}")

In CMake you need to know whether a variable is just a string, or a list of strings. The list being just a ;-separated string sounds about right. I don't think this is a proper fix though. If you take a look at the documentation of set() you would see that if you call set with tow arguments set(FOO BAR) -- you assign variable FOO string BAR, but if you do set(FOO $FOO BAR), you assign variable FOO a list of whatever $FOO evaluates to as a first element and BAR as the second element, and if you look at our code for toxcore_PKGCONFIG_LIBS:

...
  set(toxcore_PKGCONFIG_LIBS ${toxcore_PKGCONFIG_LIBS} ${CMAKE_THREAD_LIBS_INIT})
...
  set(toxcore_PKGCONFIG_LIBS ${toxcore_PKGCONFIG_LIBS} "-lrt")
...
  set(toxcore_PKGCONFIG_LIBS ${toxcore_PKGCONFIG_LIBS} "-lws2_32 -liphlpapi")

You can see that it's us who make toxcore_PKGCONFIG_LIBS be a list by using set() like this. We don't want it to be a list, so the proper way to fix this would be to not make toxcore_PKGCONFIG_LIBS a list in the first place, which is fixed by doing the following

...
  set(toxcore_PKGCONFIG_LIBS "${toxcore_PKGCONFIG_LIBS} ${CMAKE_THREAD_LIBS_INIT}")
...
  set(toxcore_PKGCONFIG_LIBS "${toxcore_PKGCONFIG_LIBS} -lrt")
...
  set(toxcore_PKGCONFIG_LIBS "${toxcore_PKGCONFIG_LIBS} -lws2_32 -liphlpapi")

(note the changed quotation marks, which make the set() functions be 2-argument, instead 3).


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 14, 2016

CMakeLists.txt, line 528 at r1 (raw file):

Previously, nurupo wrote…

In CMake you need to know whether a variable is just a string, or a list of strings. The list being just a ;-separated string sounds about right. I don't think this is a proper fix though. If you take a look at the documentation of set() you would see that if you call set with tow arguments set(FOO BAR) -- you assign variable FOO string BAR, but if you do set(FOO $FOO BAR), you assign variable FOO a list of whatever $FOO evaluates to as a first element and BAR as the second element, and if you look at our code for toxcore_PKGCONFIG_LIBS:

...
  set(toxcore_PKGCONFIG_LIBS ${toxcore_PKGCONFIG_LIBS} ${CMAKE_THREAD_LIBS_INIT})
...
  set(toxcore_PKGCONFIG_LIBS ${toxcore_PKGCONFIG_LIBS} "-lrt")
...
  set(toxcore_PKGCONFIG_LIBS ${toxcore_PKGCONFIG_LIBS} "-lws2_32 -liphlpapi")

You can see that it's us who make toxcore_PKGCONFIG_LIBS be a list by using set() like this. We don't want it to be a list, so the proper way to fix this would be to not make toxcore_PKGCONFIG_LIBS a list in the first place, which is fixed by doing the following

...
  set(toxcore_PKGCONFIG_LIBS "${toxcore_PKGCONFIG_LIBS} ${CMAKE_THREAD_LIBS_INIT}")
...
  set(toxcore_PKGCONFIG_LIBS "${toxcore_PKGCONFIG_LIBS} -lrt")
...
  set(toxcore_PKGCONFIG_LIBS "${toxcore_PKGCONFIG_LIBS} -lws2_32 -liphlpapi")

(note the changed quotation marks, which make the set() functions be 2-argument, instead 3).

s/tow arguments /two arguments/


Comments from Reviewable

@nurupo nurupo mentioned this pull request Dec 14, 2016
@iphydf
Copy link
Member Author

iphydf commented Dec 14, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


CMakeLists.txt, line 528 at r1 (raw file):

Previously, nurupo wrote…

s/tow arguments /two arguments/

I specifically chose not to do it this way. One reason is that I wasn't sure whether CMAKE_THREAD_LIBS_INIT is a list or a string (turns out it doesn't matter, because it's always exactly empty or 1 element). The more important reason is that with the proposed string concatenation instead of list building, you end up with a leading space, which either makes the output .pc or the input .pc.in file ugly.

I ended up searching google for "cmake list to space separated string" and found this. Someone commented that it doesn't work.. it does, though. There could be some issues, but I don't see how they would be resolved using a foreach loop. Issues I see are:

  • One of the strings contains an escaped ";". This is fixed by using the foreach loop, but then you end up with ";" in the pkg-config file, which would break things anyway.
  • One of the strings contains a space " ". This is not fixed using a foreach loop and would either way require some escaping. We don't do this, and simply assume that library names never contain a space (a reasonable assumption).

Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 14, 2016

:lgtm_strong:


Review status: all files reviewed at latest revision, 1 unresolved discussion.


CMakeLists.txt, line 528 at r1 (raw file):

Previously, iphydf wrote…

I specifically chose not to do it this way. One reason is that I wasn't sure whether CMAKE_THREAD_LIBS_INIT is a list or a string (turns out it doesn't matter, because it's always exactly empty or 1 element). The more important reason is that with the proposed string concatenation instead of list building, you end up with a leading space, which either makes the output .pc or the input .pc.in file ugly.

I ended up searching google for "cmake list to space separated string" and found this. Someone commented that it doesn't work.. it does, though. There could be some issues, but I don't see how they would be resolved using a foreach loop. Issues I see are:

  • One of the strings contains an escaped ";". This is fixed by using the foreach loop, but then you end up with ";" in the pkg-config file, which would break things anyway.
  • One of the strings contains a space " ". This is not fixed using a foreach loop and would either way require some escaping. We don't do this, and simply assume that library names never contain a space (a reasonable assumption).

Okay.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 14, 2016

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from Reviewable

@iphydf iphydf merged commit bb0fbf9 into TokTok:master Dec 14, 2016
@iphydf iphydf deleted the fix-pkgconfig branch December 14, 2016 09:38
This pull request was closed.
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