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

[proj4]Rename dll to proj.dll/projd.dll #6180

Closed
wants to merge 5 commits into from
Closed

[proj4]Rename dll to proj.dll/projd.dll #6180

wants to merge 5 commits into from

Conversation

JackBoosY
Copy link
Contributor

When using port gdal functions(such as OCTTransform()), we will received errors that cannot find proj.dll, so I rename release dll and debug dll.

See #3408.

@JackBoosY JackBoosY added the info:internal This PR or Issue was filed by the vcpkg team. label Apr 23, 2019
@Neumann-A
Copy link
Contributor

Neumann-A commented Apr 23, 2019

seems like the lines 42 to 69 should probably be removed from the portfile because it changes the default behavior of the package.
Furthermore you forgot to check the generated cmake target files which now have a dangling reference to the versioned dll.

@grdowns grdowns self-assigned this Apr 23, 2019
@grdowns grdowns added the wip label Apr 23, 2019
@JackBoosY
Copy link
Contributor Author

@Neumann-A thanks for advice.
In fact, the library name generated by using cmakelists.txt in proj4 is proj4_9.dll, so we need to modify its cmakelists.txt now.

@Neumann-A
Copy link
Contributor

@JackBoosY:
maybe a stupid question: Why is this renaming even required? proj4 exports targets so it should not be required to rename the library for cmake projects in any way. This would preserve the default behavior the maintainer intended (and it is intended for ABI reasons if you search for the function proj_target_output_name within the source). If it is because of gdal then the portfile in gdal must be fixed.

@JackBoosY JackBoosY removed the wip label Apr 25, 2019
@JackBoosY
Copy link
Contributor Author

JackBoosY commented Apr 25, 2019

@grdowns Please have a look.
libgeotiff was successfully built in my WM and vxl built failure with the following details(#4711):

Can't build VXL with non built-in OpenJpeg in current version. Please remove OpenJpeg, and try install VXL again.

All of these failures are not caused by proj4.

@JackBoosY
Copy link
Contributor Author

@Neumann-A In fact, I am not sure about it.
The error in #3408 should be caused by a forced change to the library name.
And you can see these changes through this link.

@Neumann-A
Copy link
Contributor

@JackBoosY #3258 says that the behavior should be changed in gdal (#3258 (comment)) and not in proj4

but a patch to gdal would be much better.

Even though renaming proj4 here would solve the problem in gdal its just a bad temporary fix. The right solution is to fix the behavior of gdal itself and where it tries to locate the dll.
If you remove the renaming, then the dll should be copied without a problem (this should solve #3408). The problem was due to the lib being renamed but the dll not being renamed. (A good example why default behavior should not be changed... you might forget something not directly turning up in CI)
You will then probably see a regression in gdal because it assumes a certain name of proj4 in the portfile. This must be changed to basically a find_package call an a extraction of correct library name from the proj4 exported targets to proof it against future changes to proj4. If gdal then still fails to load the dll then the gdal sources must corrected or the required environment variable has to be set.

@JackBoosY
Copy link
Contributor Author

@Neumann-A In fact, proj's README has written:

The makefile.vc builds proj.exe, proj.dll and proj.lib.

So here should change the library name to proj instead of changing other dependent ports.

@Neumann-A
Copy link
Contributor

@JackBoosY: Thats probably because the version of Proj4 in vcpkg is rather old (4.9.3 on 22 Aug 2016). Proj4 is already at 6.0.0 (https://github.com/OSGeo/proj.4/releases). Also the documentation seems to lag a bit behind: Although they export correct targets in cmake they still say to link against PROJ_LIBRARIES which I would consider outdated due to the existance of targets. So I wouldn't rely to much on the docs and follow what the actual code tells me. The newer readme also does not mention anything in that direction

@JackBoosY
Copy link
Contributor Author

@Neumann-A I found that people are always using proj.dll instead of proj_${VERSION}.dll, not changing the library name may cause other dependent port calls to fail.

@Neumann-A
Copy link
Contributor

@JackBoosY: How many ports in vcpkg would be affected?
On my end I don't mind about the rename too much since I don't use proj4 and even so as long as the cmake targets are correct everything will work. The only thing I mind is changing the default behavior here. Does gdal really search for proj.dll or does it search for anything that is proj and just uses whatever it has been linked to? In the latter case I am against changing dll name because the linkage just has to be corrected in the gdal port file.

@Neumann-A
Copy link
Contributor

Neumann-A commented May 24, 2019

@JackBoosY Are you sure people are using proj instead of proj_version? The problem here is that proj4 defines PROJ4_LIBRARIES as proj from its config file. This proj is not a library name but a cmake target defined by PROJ4 so renaming the library hides the true issue that someone might be missing a find_dependency(PROJ4) call in its config file. See issue #6592.

How did i find that one? One of my logs told me the following:

-- VCPKG-find_package: Value of PROJ4_BINARY_DIRS: G:/vcpkg_test/vcpkg/installed/x64-windows/bin
-- VCPKG-find_package: Value of PROJ4_CONFIG: G:/vcpkg_test/vcpkg/installed/x64-windows/share/proj4/proj4-config.cmake
-- VCPKG-find_package: Value of PROJ4_CONSIDERED_CONFIGS: G:/vcpkg_test/vcpkg/installed/x64-windows/share/proj4/proj4-config.cmake
-- VCPKG-find_package: Value of PROJ4_CONSIDERED_VERSIONS: 4.9.3
-- VCPKG-find_package: Value of PROJ4_DIR: G:/vcpkg_test/vcpkg/installed/x64-windows/share/proj4
-- VCPKG-find_package: Value of PROJ4_FOUND: 1
-- VCPKG-find_package: Value of PROJ4_INCLUDE_DIR: G:/vcpkg_test/vcpkg/installed/x64-windows/include
-- VCPKG-find_package: Value of PROJ4_INCLUDE_DIRS: G:/vcpkg_test/vcpkg/installed/x64-windows/include
-- VCPKG-find_package: Value of PROJ4_LIBRARIES: proj
-- VCPKG-find_package: Value of PROJ4_LIBRARY_DIRS: G:/vcpkg_test/vcpkg/installed/x64-windows/lib
-- VCPKG-find_package: Value of PROJ4_VERSION: 4.9.3
-- VCPKG-find_package: Value of PROJ4_VERSION_COUNT: 3
-- VCPKG-find_package: Value of PROJ4_VERSION_MAJOR: 4
-- VCPKG-find_package: Value of PROJ4_VERSION_MINOR: 9
-- VCPKG-find_package: Value of PROJ4_VERSION_PATCH: 3
-- VCPKG-find_package: Value of PROJ4_VERSION_TWEAK: 0

If you then check the targets generated by proj4 you actually find that proj is a target name

(The whole story is linker issue in #5543 in port pdal which I investigated and found libgeotiff as a culprit)

@JackBoosY
Copy link
Contributor Author

@Neumann-A This really made a decisive blow to me. Okay, I will close this PR and re-solve this issue with your solution.
Thanks.

@JackBoosY JackBoosY closed this May 24, 2019
@JackBoosY
Copy link
Contributor Author

@Neumann-A By looking at the proj4 output file, I found that it generated proj.lib and proj4_9.dll, maybe we should fix it too?
And find_package does not work in portfile.cmake, we can only set the debug/release path manually.

@Neumann-A
Copy link
Contributor

@JackBoosY The renaming from the portfile should be completely removed. Currently the portfile is renaming it to proj.lib.

And find_package does not work in portfile.cmake, we can only set the debug/release path manually.

I also found that out. What works instead is a find_library call. In PR 6658 I changed Qt to use find_library instead of hardcoded paths. Example:

set(VCPKG_RELEASE_LIBDIR "${CURRENT_INSTALLED_DIR}/lib")
set(VCPKG_DEBUG_LIBDIR "${CURRENT_INSTALLED_DIR}/debug/lib")
if(WIN32)
    set(CMAKE_FIND_LIBRARY_PREFIXES "")
    set(CMAKE_FIND_LIBRARY_SUFFIXES ".lib")
elseif(VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Linux")
    set(CMAKE_FIND_LIBRARY_PREFIXES "lib")
    set(CMAKE_FIND_LIBRARY_SUFFIXES ".so;.a")
elseif(VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Darwin")
    set(CMAKE_FIND_LIBRARY_PREFIXES "lib")
    set(CMAKE_FIND_LIBRARY_SUFFIXES ".dylib;.so;.a")
endif()
find_library(LIBPNG_LIBS_RELEASE    NAMES png16 libpng16               PATHS "${VCPKG_RELEASE_LIBDIR}" NO_DEFAULT_PATH)
find_library(LIBPNG_LIBS_DEBUG      NAMES png16d libpng16d             PATHS "${VCPKG_DEBUG_LIBDIR}"   NO_DEFAULT_PATH)

The nice thing about it you do not have to thing too much about the OS differences. unfortunately the prologue

set(VCPKG_RELEASE_LIBDIR "${CURRENT_INSTALLED_DIR}/lib")
set(VCPKG_DEBUG_LIBDIR "${CURRENT_INSTALLED_DIR}/debug/lib")
if(WIN32)
    set(CMAKE_FIND_LIBRARY_PREFIXES "")
    set(CMAKE_FIND_LIBRARY_SUFFIXES ".lib")
elseif(VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Linux")
    set(CMAKE_FIND_LIBRARY_PREFIXES "lib")
    set(CMAKE_FIND_LIBRARY_SUFFIXES ".so;.a")
elseif(VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Darwin")
    set(CMAKE_FIND_LIBRARY_PREFIXES "lib")
    set(CMAKE_FIND_LIBRARY_SUFFIXES ".dylib;.so;.a")
endif()

is required because in script mode cmake does not default them. Would be nice if vcpkg would default those values somewhere before reading in the portfile.

@JackBoosY
Copy link
Contributor Author

@Neumann-A , PR #343 please have a look.

@Neumann-A
Copy link
Contributor

Neumann-A commented May 29, 2019

@JackBoosY I don't know too much about Linux builds but instead of renaming the library adding a symlink on those platforms seems more appropriate. (if it is really required.)

@JackBoosY
Copy link
Contributor Author

JackBoosY commented May 31, 2019

@Neumann-A I don't know which way is better...
Some libraries do use the name "proj", and the name of this library is originally proj instead of proj4.
It seems that we need to be careful to modify this issue.

@Neumann-A
Copy link
Contributor

@JackBoosY: The problem here is that there is no clear guide how to handle these cases. And even the MS team seem to not have a clear idea what should be done. I can only tell you how I would handle these cases and that would be the following:

  • follow whatever find_package expects or follow whatever the package default is. Giving the default a slightly higher priority if the package is not doing something unreasonable. (The default also simply adding CMAKE_DEBUG_POSTFIX if it is a CMake projects)
    An example for unreasonable: OpenSSL is an example were I would follow CMakes find_package default to add a debug suffix and not the package default to not add it because the prebuild Windows binaries of openssl come with a debug suffix although the build itself does not add it!
  • give windows behavior priority over other os behavior.
  • if the above fails to be reasonable for some reason -> come up with a reasonable default.

In the case of proj: I would follow the package default because packages using the original(old) proj have probably not been updated to the changes to proj and need an update to the new library name. Also having a versioned library name with debug suffix is a perfectly reasonable thing to have (and is I believe quite important for dlls)
This decision also makes sure that the library maintainer decides how his library is named and not the consuming packages even if it means that it breaks those consuming packages. It also probably only breaks packages which are not using CMake so another question is if VCPKG wants to make CMake a first class citizen and put pressure on packages not using CMake to increase the adoption of CMake.

TL;DR; I have my opinion about how to solve it but I cannot tell you how to solve it. That is not something for me to decide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants