Allow to provide own locations for zlib, openssl etc #2474

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@kdekker
Contributor

kdekker commented Apr 9, 2018

Allow to provide own locations for zlib, openssl, libssh2, cares.

The link.exe /lib failed, because it added (Microsoft) dependencies. As create a static lib is just aggregating objects into an archive, use lib.exe instead.

As lib.exe does not perform optimizations nor linking, the optimizations and dependent windows libraries should not be provided as flags to the archiver.

Allow to provide own locations for zlib, openssl etc
Allow to provide own locations for zlib, openssl, libssh2, cares.

The link.exe /lib failed, because it added (Microsoft) dependencies. As create a static lib is just aggregating objects into an archive, use lib.exe instead.

As lib.exe does not perform optimizations nor linking, the optimizations and dependent windows libraries should not be provided as flags to the archiver.
@jay

This comment has been minimized.

Show comment Hide comment
@jay

jay Apr 9, 2018

Member

Ref: #1201

@kdekker the variables you're adding need to be documented

@rodwiddowson can you give feedback on this pr please

Member

jay commented Apr 9, 2018

Ref: #1201

@kdekker the variables you're adding need to be documented

@rodwiddowson can you give feedback on this pr please

@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 10, 2018

Contributor

First of all: the current approach differs from #1201. In the #1201, I added variables that manipulate CFLAGS en LFLAGS directly. In this approach, I provide e.g. ZLIB_PATH that points to a location where a include and lib subdirectory are expected. A somewhat (but that is my opinion) cleaner approach compared to previous one (but judging own changes is not an independent opinion..)

In addition to the comments mentioned in #1201, I cannot use nmake /E. That option only allowed to set environment variables, but these then need to be used/available in the makefile.

Prior to my PR, the MakefileBuild.vc only provided the WITH_DEVEL possibility. That variable lacks the possibility to take zlib, openssl, libssh2 (and what you need more) from different locations. WITH_DEVEL also requires first to copy all other includes/libs on a per user, per version basis. It is also against the common practice on UNIX/Linux, where you can provide a path to another lib/include.

As an example: if you write some C-code, using a #include directive, then the include file should be found by providing the right /I flags. Considering this way of working, the way how WITH_DEVEL works (copying instead of referencing a location) is IMO odd or special (or probably normal, based on what someone considered as 'normal', and depending on what your opinion is).

When we started using cURL (many years ago), the old nmake method also provided a possibility to define 'your own' locations for zlib, openssl etc. In our case, these libraries are stored in a read-only location. It is inefficient to rewrite our build process and need to copy (for each build, for each user) the other includes/libraries used together with cURL. Inefficient in terms of writing a different process, inefficient in terms of inflexibility if we need to test cURL with e.g. different openSSL versions.

I hope that I've convinced you for the need of the possibility to specify other locations. Please keep in mind that I did not change the default (../../deps directory).

Some additional information about my changes (the line numbers refer to the code as if my PR was applied):

  • around line 73:
    Now use lib.exe instead of link.exe /lib. The lib.exe is the Windows counterpart of the UNIX/Linux 'ar' archiver. Instead of using an undocumented link.exe /lib (I'm working with Visual Studio 2015, but did also not find any information on MSDN/microsoft.com about this command, nor link /? told about this option). Making a static library has nothing to do with optimizing or linking. That's why prior to my PR, the link.exe /lib raises warnings about optimizer flags (like /opt:ref,icf). When making an static library/archive, both optimization flags as well as Windows libraries should be omitted from the build command. These flags are solely needed when building an exe or DLL. In addition to switching to lib.exe, also $(LFLAGS) should not be passed to the $(LNK) command, when building a static library (see lines: 378, 402, 500).
  • line 102: a minor optimization. Re-use $(LFLAGS) in CURL_LFLAGS instead of specifying it twice.
  • line 121-126: if the ../../deps (or the directory specified in $(WITH_DEVEL)) does not exist, omit it from the build command.
  • line 146, : consequently use WIN_LIBS for Windows libraries. Prevent aggregating (other) static libraries in (static) libcurl.a
  • line 128-135, 173-180, 205-211, 243-250 : allow to specify per product a top location/directory that points to their include and lib sub directories respectively.
  • around line 464: removal of comments of unused release-ssh2-ssl-dll-zlib target + follow up on 8585026 where the clean target was moved, but a remaining line of comments accidentally still left over.

I hope that the above information clarifies the background of my changes. A few of them (the last bullet of previous comment) should probably get its own PR. I will try to do so next time...

With the original sources, building in it simpelst from results into warnings, that point to wrong commands. In my example below, I just extract the curl 7.59 sources, open a VS2016 x64 native command prompt and run:

nmake /f Makefile.vc mode=static DEBUG=no

That command will result in the following warnings, which are solved after my PR will be applied (due to the switch to lib.exe instead of link.exe and not passing Windows libraries to the build command):

wldap32.lib(WLDAP32.dll) : warning LNK4006: __NULL_IMPORT_DESCRIPTOR already defined in ws2_32.lib(WS2_32.dll); second definition ignored
wldap32.lib(WLDAP32.dll) : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library
advapi32.lib(ADVAPI32.dll) : warning LNK4006: __NULL_IMPORT_DESCRIPTOR already defined in ws2_32.lib(WS2_32.dll); second definition ignored
advapi32.lib(ADVAPI32.dll) : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library
Normaliz.lib(Normaliz.dll) : warning LNK4006: __NULL_IMPORT_DESCRIPTOR already defined in ws2_32.lib(WS2_32.dll); second definition ignored
Normaliz.lib(Normaliz.dll) : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library
Crypt32.lib(CRYPT32.dll) : warning LNK4006: __NULL_IMPORT_DESCRIPTOR already defined in ws2_32.lib(WS2_32.dll); second definition ignored
Crypt32.lib(CRYPT32.dll) : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library

The above warnings mean that the same symbol (from Windows system libraries) are added twice.
If wanted, I can also show the effects of using /opt (results in a warning).

@jay: I add documentation, but first like to await a response on this comments. I think I've to write some words in BUILD.WINDOW.txt and in the usage in the MakefileBuiild.vc. Is that sufficient for documentation?

Contributor

kdekker commented Apr 10, 2018

First of all: the current approach differs from #1201. In the #1201, I added variables that manipulate CFLAGS en LFLAGS directly. In this approach, I provide e.g. ZLIB_PATH that points to a location where a include and lib subdirectory are expected. A somewhat (but that is my opinion) cleaner approach compared to previous one (but judging own changes is not an independent opinion..)

In addition to the comments mentioned in #1201, I cannot use nmake /E. That option only allowed to set environment variables, but these then need to be used/available in the makefile.

Prior to my PR, the MakefileBuild.vc only provided the WITH_DEVEL possibility. That variable lacks the possibility to take zlib, openssl, libssh2 (and what you need more) from different locations. WITH_DEVEL also requires first to copy all other includes/libs on a per user, per version basis. It is also against the common practice on UNIX/Linux, where you can provide a path to another lib/include.

As an example: if you write some C-code, using a #include directive, then the include file should be found by providing the right /I flags. Considering this way of working, the way how WITH_DEVEL works (copying instead of referencing a location) is IMO odd or special (or probably normal, based on what someone considered as 'normal', and depending on what your opinion is).

When we started using cURL (many years ago), the old nmake method also provided a possibility to define 'your own' locations for zlib, openssl etc. In our case, these libraries are stored in a read-only location. It is inefficient to rewrite our build process and need to copy (for each build, for each user) the other includes/libraries used together with cURL. Inefficient in terms of writing a different process, inefficient in terms of inflexibility if we need to test cURL with e.g. different openSSL versions.

I hope that I've convinced you for the need of the possibility to specify other locations. Please keep in mind that I did not change the default (../../deps directory).

Some additional information about my changes (the line numbers refer to the code as if my PR was applied):

  • around line 73:
    Now use lib.exe instead of link.exe /lib. The lib.exe is the Windows counterpart of the UNIX/Linux 'ar' archiver. Instead of using an undocumented link.exe /lib (I'm working with Visual Studio 2015, but did also not find any information on MSDN/microsoft.com about this command, nor link /? told about this option). Making a static library has nothing to do with optimizing or linking. That's why prior to my PR, the link.exe /lib raises warnings about optimizer flags (like /opt:ref,icf). When making an static library/archive, both optimization flags as well as Windows libraries should be omitted from the build command. These flags are solely needed when building an exe or DLL. In addition to switching to lib.exe, also $(LFLAGS) should not be passed to the $(LNK) command, when building a static library (see lines: 378, 402, 500).
  • line 102: a minor optimization. Re-use $(LFLAGS) in CURL_LFLAGS instead of specifying it twice.
  • line 121-126: if the ../../deps (or the directory specified in $(WITH_DEVEL)) does not exist, omit it from the build command.
  • line 146, : consequently use WIN_LIBS for Windows libraries. Prevent aggregating (other) static libraries in (static) libcurl.a
  • line 128-135, 173-180, 205-211, 243-250 : allow to specify per product a top location/directory that points to their include and lib sub directories respectively.
  • around line 464: removal of comments of unused release-ssh2-ssl-dll-zlib target + follow up on 8585026 where the clean target was moved, but a remaining line of comments accidentally still left over.

I hope that the above information clarifies the background of my changes. A few of them (the last bullet of previous comment) should probably get its own PR. I will try to do so next time...

With the original sources, building in it simpelst from results into warnings, that point to wrong commands. In my example below, I just extract the curl 7.59 sources, open a VS2016 x64 native command prompt and run:

nmake /f Makefile.vc mode=static DEBUG=no

That command will result in the following warnings, which are solved after my PR will be applied (due to the switch to lib.exe instead of link.exe and not passing Windows libraries to the build command):

wldap32.lib(WLDAP32.dll) : warning LNK4006: __NULL_IMPORT_DESCRIPTOR already defined in ws2_32.lib(WS2_32.dll); second definition ignored
wldap32.lib(WLDAP32.dll) : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library
advapi32.lib(ADVAPI32.dll) : warning LNK4006: __NULL_IMPORT_DESCRIPTOR already defined in ws2_32.lib(WS2_32.dll); second definition ignored
advapi32.lib(ADVAPI32.dll) : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library
Normaliz.lib(Normaliz.dll) : warning LNK4006: __NULL_IMPORT_DESCRIPTOR already defined in ws2_32.lib(WS2_32.dll); second definition ignored
Normaliz.lib(Normaliz.dll) : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library
Crypt32.lib(CRYPT32.dll) : warning LNK4006: __NULL_IMPORT_DESCRIPTOR already defined in ws2_32.lib(WS2_32.dll); second definition ignored
Crypt32.lib(CRYPT32.dll) : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library

The above warnings mean that the same symbol (from Windows system libraries) are added twice.
If wanted, I can also show the effects of using /opt (results in a warning).

@jay: I add documentation, but first like to await a response on this comments. I think I've to write some words in BUILD.WINDOW.txt and in the usage in the MakefileBuiild.vc. Is that sufficient for documentation?

@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 11, 2018

Contributor

@jay:
Q1: Do you like the documentation first or do you first perform a review?
Q2: See also previous comments: where do I need to write the documentation. Is updating the 2 mentioned files sufficient?

Contributor

kdekker commented Apr 11, 2018

@jay:
Q1: Do you like the documentation first or do you first perform a review?
Q2: See also previous comments: where do I need to write the documentation. Is updating the 2 mentioned files sufficient?

@jay

This comment has been minimized.

Show comment Hide comment
@jay

jay Apr 11, 2018

Member

Please add the documentation in Makefile.vc and BUILD.WINDOWS. Add it at the end of section Building with Visual C++ (that section is basically mirrored in the makefile as usage). The users need to know the variables are available. At any time we'd welcome a review from one of our contributors who uses winbuild regularly. Also why did you remove release-ssh2-ssl-dll-zlib special section, was it unnecessary?

Member

jay commented Apr 11, 2018

Please add the documentation in Makefile.vc and BUILD.WINDOWS. Add it at the end of section Building with Visual C++ (that section is basically mirrored in the makefile as usage). The users need to know the variables are available. At any time we'd welcome a review from one of our contributors who uses winbuild regularly. Also why did you remove release-ssh2-ssl-dll-zlib special section, was it unnecessary?

@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 11, 2018

Contributor

I will do so, and send an additional commit on my own repository (that will become visible automatically here if I understood the process) in a while (as soon as I've finished the change).

Removal of the release-ssh2-ssl-dll-zlib section was indeed not needed. I just saw commented out lines that did not match (IMO) with the remainder of the structure of the makefile. I did not see the benefits, and probably removed it too quickly. This is common practice in our own version control system, where 'code that is not used' is usually removed. Code that is not used, increases complexity and makes understanding more difficult (that's is also why I asked in another mail about dropping visual studio 6). Any code that exists, has a price. You will get nothing for free...But if you like that I leave this commented out lines, I will re-add them. However, some few words why these lines should exist, is probably a good idea.

Contributor

kdekker commented Apr 11, 2018

I will do so, and send an additional commit on my own repository (that will become visible automatically here if I understood the process) in a while (as soon as I've finished the change).

Removal of the release-ssh2-ssl-dll-zlib section was indeed not needed. I just saw commented out lines that did not match (IMO) with the remainder of the structure of the makefile. I did not see the benefits, and probably removed it too quickly. This is common practice in our own version control system, where 'code that is not used' is usually removed. Code that is not used, increases complexity and makes understanding more difficult (that's is also why I asked in another mail about dropping visual studio 6). Any code that exists, has a price. You will get nothing for free...But if you like that I leave this commented out lines, I will re-add them. However, some few words why these lines should exist, is probably a good idea.

Documented xxx_PATH variables
Some new variables have been introduced to tell locations of dependencies, e.g. for OpenSSL.

Also removed a dangling URL that no longer worked. I was not able to find the IDN download at MSDN/microsoft.com, so it seems to be removed...
@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 11, 2018

Contributor

Filed a new commit that added the requested documentation. Will this commit (d4382e9) automatically become part of this PR? (I'm still not knowing enough of git to know this).

Also removed an URL (for IDN libs) that seems no longer to exist at MSDN/microsoft.com.
I've found a base page about IDN (https://msdn.microsoft.com/en-us/library/windows/desktop/dd318142(v=vs.85).aspx), but the download link provided there is also dangling.

Contributor

kdekker commented Apr 11, 2018

Filed a new commit that added the requested documentation. Will this commit (d4382e9) automatically become part of this PR? (I'm still not knowing enough of git to know this).

Also removed an URL (for IDN libs) that seems no longer to exist at MSDN/microsoft.com.
I've found a base page about IDN (https://msdn.microsoft.com/en-us/library/windows/desktop/dd318142(v=vs.85).aspx), but the download link provided there is also dangling.

@jay

This comment has been minimized.

Show comment Hide comment
@jay

jay Apr 12, 2018

Member

Will this commit (d4382e9) automatically become part of this PR? (I'm still not knowing enough of git to know this).

Yes since you pushed it to the branch associated with this PR it is seen in the PR. Ideally something like this you'd squash it or amend it to the previous commit and then force push.

What happens if any of the _PATH options are used with WITH_DEVEL? can we just make them one liners like

ZLIB_PATH=<path to zlib>     - Custom zlib devel path
etc

Member

jay commented Apr 12, 2018

Will this commit (d4382e9) automatically become part of this PR? (I'm still not knowing enough of git to know this).

Yes since you pushed it to the branch associated with this PR it is seen in the PR. Ideally something like this you'd squash it or amend it to the previous commit and then force push.

What happens if any of the _PATH options are used with WITH_DEVEL? can we just make them one liners like

ZLIB_PATH=<path to zlib>     - Custom zlib devel path
etc

@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 12, 2018

Contributor

If one of the _PATH variants is used WITH_DEVEL, only the xxx_PATH is used for product xxx and the remainder will use the default, of course respecting the value provided with WITH_DEVEL.

In terms of code:

!IFNDEF WITH_DEVEL
WITH_DEVEL   = ../../deps
!ENDIF
DEVEL_INCLUDE= $(WITH_DEVEL)/include
DEVEL_LIB    = $(WITH_DEVEL)/lib

combined with zlib as example

!IFDEF ZLIB_PATH
ZLIB_INC_DIR = $(ZLIB_PATH)\include
ZLIB_LIB_DIR = $(ZLIB_PATH)\lib
ZLIB_LFLAGS  = $(ZLIB_LFLAGS) "/LIBPATH:$(ZLIB_LIB_DIR)"
!ELSE
ZLIB_INC_DIR = $(DEVEL_INCLUDE)
ZLIB_LIB_DIR = $(DEVEL_LIB)
!ENDIF

will probably better explain my answer. As you can see, the one is set, and the xxx_PATH overrides, but only when provided. I don't understand how this can be reduced to one liners? We need both a path to include as well as to a library. And these need to be used in compiler and/or linker flags if a dedicate _PATH value was provided. If all includes and libs are in the WITH_DEVEL location (don't make difference if this value has its default value, or got a user specified value), then next trick is used to provide a single include and linker path (and thus, these need not to be speficied again in the else branch of the above piece of code):

!IF EXISTS("$(DEVEL_INCLUDE)")
CFLAGS       = $(CFLAGS) /I"$(DEVEL_INCLUDE)"
!ENDIF
!IF EXISTS("$(DEVEL_LIB)")
LFLAGS       = $(LFLAGS) "/LIBPATH:$(DEVEL_LIB)"
!ENDIF
Contributor

kdekker commented Apr 12, 2018

If one of the _PATH variants is used WITH_DEVEL, only the xxx_PATH is used for product xxx and the remainder will use the default, of course respecting the value provided with WITH_DEVEL.

In terms of code:

!IFNDEF WITH_DEVEL
WITH_DEVEL   = ../../deps
!ENDIF
DEVEL_INCLUDE= $(WITH_DEVEL)/include
DEVEL_LIB    = $(WITH_DEVEL)/lib

combined with zlib as example

!IFDEF ZLIB_PATH
ZLIB_INC_DIR = $(ZLIB_PATH)\include
ZLIB_LIB_DIR = $(ZLIB_PATH)\lib
ZLIB_LFLAGS  = $(ZLIB_LFLAGS) "/LIBPATH:$(ZLIB_LIB_DIR)"
!ELSE
ZLIB_INC_DIR = $(DEVEL_INCLUDE)
ZLIB_LIB_DIR = $(DEVEL_LIB)
!ENDIF

will probably better explain my answer. As you can see, the one is set, and the xxx_PATH overrides, but only when provided. I don't understand how this can be reduced to one liners? We need both a path to include as well as to a library. And these need to be used in compiler and/or linker flags if a dedicate _PATH value was provided. If all includes and libs are in the WITH_DEVEL location (don't make difference if this value has its default value, or got a user specified value), then next trick is used to provide a single include and linker path (and thus, these need not to be speficied again in the else branch of the above piece of code):

!IF EXISTS("$(DEVEL_INCLUDE)")
CFLAGS       = $(CFLAGS) /I"$(DEVEL_INCLUDE)"
!ENDIF
!IF EXISTS("$(DEVEL_LIB)")
LFLAGS       = $(LFLAGS) "/LIBPATH:$(DEVEL_LIB)"
!ENDIF
@jay

This comment has been minimized.

Show comment Hide comment
@jay

jay Apr 12, 2018

Member

Ok. I meant reduce the documentation for each variable to a one liner like in my example.

Member

jay commented Apr 12, 2018

Ok. I meant reduce the documentation for each variable to a one liner like in my example.

@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 12, 2018

Contributor

Ah, I did not understand that. But keep in mind that the _PATH variables were only added for 4 libraries. Not for all stuff (like WITH_NGHTTP2 and some others). When reducing the documentation to one line, it is still useful which libraries has a _PATH variable, isn't it?

Contributor

kdekker commented Apr 12, 2018

Ah, I did not understand that. But keep in mind that the _PATH variables were only added for 4 libraries. Not for all stuff (like WITH_NGHTTP2 and some others). When reducing the documentation to one line, it is still useful which libraries has a _PATH variable, isn't it?

@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 16, 2018

Contributor

@jay: Do I need to provide more information? Is the explanation clear, or do I need to change more things. I would really like to know whether this PR will be merged or whether I need to do something more.

Contributor

kdekker commented Apr 16, 2018

@jay: Do I need to provide more information? Is the explanation clear, or do I need to change more things. I would really like to know whether this PR will be merged or whether I need to do something more.

@jay

This comment has been minimized.

Show comment Hide comment
@jay

jay Apr 16, 2018

Member

I think we should support this for nghttp2 and mbedtls as well that way all the WITH_ options have a _PATH. For the doc what do you think about this:

CARES_PATH=<path to c-ares>  - Custom path for c-ares.
SSH2_PATH=<path to libSSH2>  - Custom path for libSSH2.
SSL_PATH=<path to OpenSSL>   - Custom path for OpenSSL.
ZLIB_PATH=<path to zlib>     - Custom path for zlib.
Member

jay commented Apr 16, 2018

I think we should support this for nghttp2 and mbedtls as well that way all the WITH_ options have a _PATH. For the doc what do you think about this:

CARES_PATH=<path to c-ares>  - Custom path for c-ares.
SSH2_PATH=<path to libSSH2>  - Custom path for libSSH2.
SSL_PATH=<path to OpenSSL>   - Custom path for OpenSSL.
ZLIB_PATH=<path to zlib>     - Custom path for zlib.
@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 16, 2018

Contributor

Perfect. Shall I make that change and make a commit in my own repo (for BUILD.WINDOWS.txt + usage information in makefile)?

If wanted, I can make the nghttp2 and mbedtls changes as well (in the makefile + documentation), but I'm not known with these. So testing becomes difficult for me. Who can test my changes then?

I already discovered that there is some inconsistency between e.g. USE_SSL and USE_NGHTTP2. In the first case, USE_SSL remains undef if not set, while USE_NGHTTP2 becomes 'false' (in Makefile.vc) + no /I directive is ever used for NGHTTP2. So additional testing is really needed, but actually out of my scope.

Contributor

kdekker commented Apr 16, 2018

Perfect. Shall I make that change and make a commit in my own repo (for BUILD.WINDOWS.txt + usage information in makefile)?

If wanted, I can make the nghttp2 and mbedtls changes as well (in the makefile + documentation), but I'm not known with these. So testing becomes difficult for me. Who can test my changes then?

I already discovered that there is some inconsistency between e.g. USE_SSL and USE_NGHTTP2. In the first case, USE_SSL remains undef if not set, while USE_NGHTTP2 becomes 'false' (in Makefile.vc) + no /I directive is ever used for NGHTTP2. So additional testing is really needed, but actually out of my scope.

Allow to provide custom path to NGHTTP2 and/or MBEDTLS
As requested by Jay Satiro: added similar possibility for remaining 2 left-overs (NGHTTP/2 and mbedTLS) similar to was done before for zlib, openssl, libssb2, cares.

Also applied proposed change to the documentation (including the 2 new xxx_PATH possibilities).
@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 16, 2018

Contributor

Made a commit. Makefile checked on syntax correctness, but did not build with NGHTTP/2 and/or mbedTLS.

Contributor

kdekker commented Apr 16, 2018

Made a commit. Makefile checked on syntax correctness, but did not build with NGHTTP/2 and/or mbedTLS.

@rodwiddowson

Data point. This file breaks my curl build as it currently stands,

I'm investigating.

I cannot use the new features since I also need control over the lib names which this change doesn't offer.

I'd appreciate it if we didn't get this into mainline until I u nderstand fully and either decide my build it bhroken or we can work out what will work.

My Breaking command line:

Nmake /f Makefile.vc mode=dll WITH_DEVEL=H:\Perforce\VS2017\openssl-1.1.0g\x86
WITH_SSL=dll WITH_ZLIB=dll ENABLE_WINSSL=no VC=15 MAKE="NMAKE e"
ZLIB_LFLAGS=/libpath:H:\Perforce\VS2017\zlib-1.2.11.\Release ZLIB_CFLAGS="/D
HAVE_ZLIB_H /DHAVE_ZLIB /DHAVE_LIBZ /IH:\Perforce\VS2017\zlib-1.2.11"
ZLIB_LIBS=zlib1.lib SSL_LIBS="libcrypto.lib libssl.lib" BASE_NAME=libcurl7_58
BASE_NAME_DEBUG=libcurl7_58d

@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 16, 2018

Contributor

@rodwiddowson: please show your directory structure and involved library names. There are a few IF EXISTS() constructs, e.g. to determine the naming of OpenSSL, but these existed before. I added a similar construct for zlib.

Do you use your own lib names, or are the used lib names default? Why is the check for e.g. openSSL not smart enough for you? Library names do not change that often (hopefully)?

Based on your build flags: the names of the openSSL libraries should be recognized, but will fail if you store multiple versions of OpenSSL in the WITH_DEVEL directory. Instead of it, use dedicate output directories for OpenSSL, and use SSL_PATH.

For the output result of cURL (you are using non-default names for BASE_NAME and BASE_NAME_DEBUG), I don't see a relationship with my changes...

So: for which library names do you need control for? Anything else than cURL is IMO 'dicatated' by the 'other-product' maintainers. And the cURL output itself can be controlled, as it could before. Or am I wrong?

Contributor

kdekker commented Apr 16, 2018

@rodwiddowson: please show your directory structure and involved library names. There are a few IF EXISTS() constructs, e.g. to determine the naming of OpenSSL, but these existed before. I added a similar construct for zlib.

Do you use your own lib names, or are the used lib names default? Why is the check for e.g. openSSL not smart enough for you? Library names do not change that often (hopefully)?

Based on your build flags: the names of the openSSL libraries should be recognized, but will fail if you store multiple versions of OpenSSL in the WITH_DEVEL directory. Instead of it, use dedicate output directories for OpenSSL, and use SSL_PATH.

For the output result of cURL (you are using non-default names for BASE_NAME and BASE_NAME_DEBUG), I don't see a relationship with my changes...

So: for which library names do you need control for? Anything else than cURL is IMO 'dicatated' by the 'other-product' maintainers. And the cURL output itself can be controlled, as it could before. Or am I wrong?

@rodwiddowson

This comment has been minimized.

Show comment Hide comment
@rodwiddowson

rodwiddowson Apr 16, 2018

Contributor

OK. My PEBKAC confirmed. I'm fine with this change. Sorry for the noise.

I may need to do some cleanup of our build processes to take full advantage of this (probably by adding an install target to our zlib build), but thats my problem, not libCurl.

Once again sorry about the noise.

Contributor

rodwiddowson commented Apr 16, 2018

OK. My PEBKAC confirmed. I'm fine with this change. Sorry for the noise.

I may need to do some cleanup of our build processes to take full advantage of this (probably by adding an install target to our zlib build), but thats my problem, not libCurl.

Once again sorry about the noise.

@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 16, 2018

Contributor

LOL. Learning a new word (PEBKAC) 👍 (English is not my native language)

Contributor

kdekker commented Apr 16, 2018

LOL. Learning a new word (PEBKAC) 👍 (English is not my native language)

@jay jay closed this in 7921659 Apr 17, 2018

@jay

This comment has been minimized.

Show comment Hide comment
@jay

jay Apr 17, 2018

Member

Thanks

Member

jay commented Apr 17, 2018

Thanks

@kdekker

This comment has been minimized.

Show comment Hide comment
@kdekker

kdekker Apr 17, 2018

Contributor

Thanks Jay (and all others that were involved).

Contributor

kdekker commented Apr 17, 2018

Thanks Jay (and all others that were involved).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment