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

set MinGW LIBS in configure.ac, not in Makefile.am #841

Closed
wants to merge 1 commit into from

Conversation

starius
Copy link

@starius starius commented Apr 24, 2016

Fix libuv.pc on MinGW.

Previous version generates libuv.pc with LIBS = "-lpthread" instead of "-lpthread -lws2_32 -lpsapi -liphlpapi -lshell32 -luserenv" on MinGW.

Previous version generates the following Makefile.am:

am__append_4 = -lws2_32 -lpsapi -liphlpapi -lshell32 -luserenv
...
LIBS = -lpthread  $(am__append_4)

and the following configure.ac:

AS_IF([test "x$PKG_CONFIG" != "x"], [
    AC_CONFIG_FILES([libuv.pc])
])
AC_CONFIG_FILES([Makefile])

Therefore, Makefile is generated after libuv.pc. Variables from Makefile don't affect config.status and libuv.pc.

Tony Theodore proposed to set LIBS in configure.ac instead of Makefile.am and created this patch.

See also mxe/mxe#1291

Fix libuv.pc on MinGW.

Previous version generates libuv.pc with LIBS = "-lpthread" instead of
"-lpthread -lws2_32 -lpsapi -liphlpapi -lshell32 -luserenv" on MinGW.

Previous version generates the following Makefile.am:

    am__append_4 = -lws2_32 -lpsapi -liphlpapi -lshell32 -luserenv
    ...
    LIBS = -lpthread  $(am__append_4)

and the following configure.ac:

    AS_IF([test "x$PKG_CONFIG" != "x"], [
        AC_CONFIG_FILES([libuv.pc])
    ])
    AC_CONFIG_FILES([Makefile])

Therefore, Makefile is generated after libuv.pc. Variables from
Makefile don't affect config.status and libuv.pc.

Tony Theodore proposed to set LIBS in configure.ac instead of
Makefile.am and created this patch.

See also mxe/mxe#1291
@starius starius mentioned this pull request Apr 24, 2016
@saghul
Copy link
Member

saghul commented Apr 25, 2016

What is the rationale behind this? I can compile libuv on MinGW just fine.

@bnoordhuis
Copy link
Member

I think the idea is to get the list of libraries into the Libs: field in libuv.pc.in.

@starius Is this patch intended for linking against the static library? I would expect the shared library to pull in the requisite dependencies automatically.

@starius
Copy link
Author

starius commented Apr 25, 2016

@saghul, this patch is needed to fix Libs in libuv.pc, as @bnoordhuis said.
@bnoordhuis, the patch is not needed on shared targets.

@saghul
Copy link
Member

saghul commented Apr 25, 2016

Verified the change: https://www.dropbox.com/s/mvk86birei1okjc/Screenshot%202016-04-25%2018.06.29.png?dl=0

LGTM

Now, should we apply the same principle to other platforms too?

PS: @starius I just reread my first message, sorry about that, I sound like an asshole there :-S

@starius
Copy link
Author

starius commented Apr 25, 2016

Now, should we apply the same principle to other platforms too?

I have not found other platform specific additions to LIBS:

$ git grep -w LIBS
configure.ac:    LIBS="$LIBS -lws2_32 -lpsapi -liphlpapi -lshell32 -luserenv"
libuv.pc.in:Libs: -L${libdir} -luv @LIBS@

saghul pushed a commit that referenced this pull request May 5, 2016
Instead of doing it instead Makefile.am, this fixes libuv.pc on MinGW.

Previous version generates libuv.pc with LIBS = "-lpthread" instead of
"-lpthread -lws2_32 -lpsapi -liphlpapi -lshell32 -luserenv" on MinGW.

Previous version generates the following Makefile.am:

    am__append_4 = -lws2_32 -lpsapi -liphlpapi -lshell32 -luserenv
    ...
    LIBS = -lpthread  $(am__append_4)

and the following configure.ac:

    AS_IF([test "x$PKG_CONFIG" != "x"], [
        AC_CONFIG_FILES([libuv.pc])
    ])
    AC_CONFIG_FILES([Makefile])

Therefore, Makefile is generated after libuv.pc. Variables from
Makefile don't affect config.status and libuv.pc.

PR-URL: #841
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@saghul
Copy link
Member

saghul commented May 5, 2016

Landed in 9386f2c, thanks!

@saghul saghul closed this May 5, 2016
kthelgason pushed a commit to kthelgason/libuv that referenced this pull request May 7, 2016
Instead of doing it instead Makefile.am, this fixes libuv.pc on MinGW.

Previous version generates libuv.pc with LIBS = "-lpthread" instead of
"-lpthread -lws2_32 -lpsapi -liphlpapi -lshell32 -luserenv" on MinGW.

Previous version generates the following Makefile.am:

    am__append_4 = -lws2_32 -lpsapi -liphlpapi -lshell32 -luserenv
    ...
    LIBS = -lpthread  $(am__append_4)

and the following configure.ac:

    AS_IF([test "x$PKG_CONFIG" != "x"], [
        AC_CONFIG_FILES([libuv.pc])
    ])
    AC_CONFIG_FILES([Makefile])

Therefore, Makefile is generated after libuv.pc. Variables from
Makefile don't affect config.status and libuv.pc.

PR-URL: libuv#841
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
starius added a commit to LuaAndC/mxe that referenced this pull request May 29, 2016
The patches are not needed:

  * libuv/libuv#820
  * libuv/libuv#841
@TimothyGu
Copy link

TimothyGu commented May 30, 2016

Actually technically, the static-only libraries should be specified in Libs.private line of the pkg-config file, which is used only when static linking is used.

(Edit) There are no real technical negative repercussions for explicitly linking the libraries for shared usage, but when one inspect the final linked binary with tools like objdump -x the noisy libraries will show up, so it's usually considered good practice to put them in .private.

@starius
Copy link
Author

starius commented May 30, 2016

@TimothyGu, I created new issue for that #895

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.

5 participants