Open jvbsl opened 1 year ago
@jvbsl For the renaming of the lib, it is possible that it is caused by a conflict in the lib name provided by some ports. I need to investigate it. :)
You mean whether another port produces a library with the same name? I doubt that, because then there would be a collision with the shared lib as well. Otherwise I don't exactly get what you mean^^
I think this location is more relevant: https://github.com/zeroc-ice/ice/blob/badafe8b848181a720ab96b48da6abc170fd918c/cpp/msbuild/ice.cpp.props#L257 That's what I meant when I wrote about ice.cpp.props, sorry for the confusion.
The thing is, yes it is upstream behaviour, but only when using the vcxproj msbuild. When using cmake it is different. But apart from that imho it should not matter for VCPKG, because no matter the library name I think cmake using vcpkg should be able to find the correct library. That's why I asked whether to patch the name to contain the version(so that the name matches what vcpkg and cmake are currently looking for), or to change what vcpkg is looking for in case of a static library(which I do not know how to do)
AFAIK, vcpkg does not provide the find_package() for zeroc-ice
.
I mean we built the libs through the upstream vcxproj
, but we did not provide any cmake methods to use these libs directly.
The cmake method using find_path
or find_library
should not have the problem you describe.
Alright, that does make sense. But my guess would be that FindIce.cmake uses the naming scheme you get when you build Ice using cmake instead of using the vcxproj. So we could either patch our vcxproj to use the same naming, perhaps even get it upstream, or we could use cmake directly instead(would probably be a lot more work, and if not careful could result in other unpredictable problems).
I opened a issue in the zeroc-ice repo: https://github.com/zeroc-ice/ice/issues/1502
It is very difficult to discover from this discussion:
CMake comes with an official FindIce.cmake
module (since CMake 3.1!),
for find_package(Ice)
,
but it doesn't work with vcpkg's port zeroc-ice
.
Well, this is what vcpkg-cmake-wrapper.cmake
exist for... but it doesn't exist yet for this port.
but it works for me, for x64-windows, x64-linux, x64-osx, x64-osx-dynamic(haven't tried x64-linux-dynamic), only x64-windows-static doesn't find it. And as far as I understood now that would use the official FindIce.cmake, right?
but it works for me, for x64-windows, x64-linux, x64-osx, x64-osx-dynamic(haven't tried x64-linux-dynamic), only x64-windows-static doesn't find it.
Does it correctly find debug libraries? (I don't know.)
And as far as I understood now that would use the official FindIce.cmake, right?
Assuming that "that" means find_package(Ice)
, it will normally use the official CMake module.
Well totally forgot: -DIce_HOME='C:/vcpkg/installed/x64-windows' -DIce_SLICE_DIR='C:/vcpkg/installed/x64-windows/share/ice/slice'
I'm setting these variables therefore it is found(so it does not find debug libs, but only release libs). I'm sorry about that, I forgot I had these as I was migrating from a custom port, which needed these and I set them a few months ago in a file I forgot about^^
That of course changes things quite a bit I guess.
So that means actually all library searches are currently broken, and I just had a workaround for that in place?
And what actually would need to be done is the implementation of a vcpkg-cmake-wrapper.cmake
file?
So that means actually all library searches are currently broken, and I just had a workaround for that in place?
Yes. It is good that you reported/confirmed your workaround. This is something which the wrapper could do.
And what actually would need to be done is the implementation of a
vcpkg-cmake-wrapper.cmake
file?
It would need to set input/cache variables before calling into the actual Find module, e.g. Ice_HOME
or ..._LIBRARY_DEBUG
.
And it may need to add stuff to the Find module's output, such as additional link libraries for static linkage.
Sometimes there is also some polyfill for different versions of CMake.
See ports zlib and tiff for examples. I don't think it is a beginner's task. I even added a test port to protect against regressions for major modules.
This what I get from find_package(Ice)
on x64-osx:
-- Failed to find all Ice components (missing: Ice_SLICE_DIR Ice_LIBRARY) (found version "3.7.9")
Relevant stderr:
find_path called with the following settings:
VAR: Ice_SLICE_DIR
NAMES: "Ice/Connection.ice"
Documentation: Ice slice directory
(It is available in share/ice
.)
Hm, the last CI builds of #33648 indicate that FindIce.cmake
is a little special.
With these prerequisites, user project configuration succeeds in vcpkg CI, also for static x64-windows triplets:
https://dev.azure.com/vcpkg/public/_build/results?buildId=94050&view=results
Damn you are fast :D I was experimenting as well, as I hoped to be able to help at least a little.
- You must supply some component(s).
Should be fine, if the normal FindIce already does it that way, right?
- You must enable CXX for MSVC.
Don't exactly get what you mean with that. Do you mean it can't be linked in a C project? That would be kinda interesting as I thought, you could even link a cpp library into a C project anyway, as it just needs to be linked to the Cpp runtime as well. But should be fine as well, right?
Apart from that the only interesting thing(which probably is vcpkg standard behaviour) that I can add is that Ice_ICE++11_LIBRARY_RELEASE
and Ice_ICE++11_LIBRARY_DEBUG
are populated with the same library(the debug version in a debug build, and the release version in the release build). I mean it works, but I personally kinda expected Ice_ICE++11_LIBRARY_RELEASE
to be always the release library and Ice_ICE++11_LIBRARY_DEBUG
always being the debug library as well as Ice_ICE++11_LIBRARY
to be the debug or relase library dependent on CMAKE_BUILD_TYPE
.
Still, anything I could help with?
- You must supply some component(s).
Should be fine, if the normal FindIce already does it that way, right?
My point is that the "normal FindIce" does require components, which is unusual.
find_package(Ice REQUIRED) # always fails, unexpectedly
find_package(Ice REQUIRED COMPONENTS Ice++11) # works
- You must enable CXX for MSVC.
Don't exactly get what you mean with that. Do you mean it can't be linked in a C project? That would be kinda interesting as I thought, you could even link a cpp library into a C project anyway, as it just needs to be linked to the Cpp runtime as well. But should be fine as well, right?
If you want the MSVC libs with their long names to be found, you need CMAKE_CXX_COMPILER_ID
or CMAKE_CXX_SIMULATE_ID
. These variables are not initialized if a project is configured to use C only.
Cf. https://github.com/Kitware/CMake/blob/7f5d5f6e5aa2ab4c7043756b607125154fe44666/Modules/FindIce.cmake#L452C11-L471
Apart from that the only interesting thing(which probably is vcpkg standard behaviour) that I can add is that
Ice_ICE++11_LIBRARY_RELEASE
andIce_ICE++11_LIBRARY_DEBUG
are populated with the same library(the debug version in a debug build, and the release version in the release build). I mean it works, but I personally kinda expectedIce_ICE++11_LIBRARY_RELEASE
to be always the release library andIce_ICE++11_LIBRARY_DEBUG
always being the debug library as well asIce_ICE++11_LIBRARY
to be the debug or relase library dependent onCMAKE_BUILD_TYPE
.
Given the debug postfix d
, Ice_ICE++11_LIBRARY_RELEASE/_DEBUG
should be properly set to debug vs. release, and Ice_ICE++11_LIBRARY
should combine them using optimized/debug
keywords. I need to check this once more.
In any case, external validation of #33648 is welcome. I don't have VS, I can only check vcpkg CI logs.
My point is that the "normal FindIce" does require components, which is unusual.
Yeah but as it is unusual behaviour already on the original library side of things, imho we do not need to work around that, or do we?
If you want the MSVC libs with their long names to be found, you need
CMAKE_CXX_COMPILER_ID
orCMAKE_CXX_SIMULATE_ID
. These variables are not initialized if a project is configured to use C only.
I didn't even know one could do that I mean most modern C compilers are afaik just the C++ compilers with restrictions(could be wrong though). But then again that it does not work with a C project is a FindIce quirk, which isn't our responsibility, right?
Given the debug postfix
d
,Ice_ICE++11_LIBRARY_RELEASE/_DEBUG
should be properly set to debug vs. release, andIce_ICE++11_LIBRARY
should combine them usingoptimized/debug
keywords. I need to check this once more.
Sorry, I think I wasn't too clear there, the test I did was on linux not on windows, and that does afaik not use a d
-suffix and therefore the ordering of the search paths does change the result for both release and debug library search.
In any case, external validation of #33648 is welcome. I don't have VS, I can only check vcpkg CI logs.
Yeah, I'll try to validate tomorrow. But for me it is also either linux natively, or CI(using tmate, often helps a lot), as well as a VM(on my ultra modern dual core CPU :D)
x64-windows looks good:
-- Ice_LIBRARIES: optimized;D:/installed/x64-windows/lib/ice37++11.lib;debug;D:/installed/x64-windows/debug/lib/ice37++11d.lib
but x64-windows-static is like you noticed:
-- Ice_LIBRARIES: D:/installed/x64-windows-static/debug/lib/ice++11.lib
I wanted to try it inside a real project, but I can't build the static version because of the dependencies I need, but I looked into the dependencies and have a quick programmatically collected json of the dependencies
And I would conclude that glacier2lib
, iceboxlib
, icessl
and icestormlib
aren't needed features of icediscovery
and icelocatordiscovery
.
Don't know if I should move this to a different issue instead? But I thought, because you are already doing a big PR which changes the build in a big way already, that it still kinda fits here
a quick programmatically collected json of the dependencies
Nice. I will try to pick it up.
x64-windows-static is like you noticed:
-- Ice_LIBRARIES: D:/installed/x64-windows-static/debug/lib/ice++11.lib
but now:
-- Ice_LIBRARIES: optimized;D:/installed/x64-windows-static/lib/ice++11.lib;debug;D:/installed/x64-windows-static/debug/lib/ice++11.lib
Oh so you are now actually trying to search yourself. With my testing(not including the two commits from 5 hours ago, will do so now). I've actually got another interesting problem, which could perhaps even be a problem of zeroc-ice itself and not of this port.
I link to installed/x64-windows-static-md/debug/lib/ice++11.lib
only, I do not use Ice::Ice++11
but instead ${Ice_ICE++11_LIBRARY}
to be sure, that it uses the path I would expect, and it is, but while linking it tries to link to ice37++11d.lib
which does not make that much sense to me, that ice++11.lib
itself is searching for ice37++11d.lib
.
dumpbin /DEPENDENTS ice++11.lib
does not contain anything really. dumpbin /HEADERS ice++11.lib
does not contain ice37(case insensitive) anywhere.
Then I just removed a simple #include <Ice/Protocol.h>
, then it links successfully. On Windows zeroc-ice uses #pragma comment(lib, "ice37++11")
, which means it can't work, because the static library that gets built does not contain the version. So either we have a way to tell it that ice37++11
and ice++11
are equivalent(do not know of any way), or we need to workaround the naming for the static library, I already know how we could do that, should I open a PR with a patch on your PR?
On Windows zeroc-ice uses
#pragma comment(lib, "ice37++11")
AFAICS this is generated via the ICE_LIBNAME
macro in IceUtil/Config.h
. We must ensure that it generates the right name for the triplet. To get rid of 37
, we might replace NAME ICE_SO_VERSION
with NAME
.
As the cmake build of zeroc-ice generates ice37++11.lib even for the static build, and zeroc-ice project plans to fix this to use the name with the version in 3.7.10 as well https://github.com/zeroc-ice/ice/issues/1502, I think it would make more sense to just patch the vcxproj to generate the static library with the same name as well, then, wen 3.7.10 comes around we would only need to remove the patch file, and the library search does not need to be changed and is uniform across all configurations. What do you think?
We shouldn't change names of binaries in 3.7.9. In particular when upstream isn't interested in supporting static windows builds. (IMO they also don't care enough for static non-windows builds where users need to care for link order.)
the cmake build of zeroc-ice
???
We shouldn't change names of binaries in 3.7.9. In particular when upstream isn't interested in supporting static windows builds. (IMO they also don't care enough for static non-windows builds where users need to care for link order.)
Well for non windows, it just works for me. And for windows it is sooo close and as far as I understood, they at least care enough to fix the naming with 3.7.10.
???
Uff, now I'm confused I was so sure I used cmake on zeroc-ice. But there are no cmake files, what the hell did I do then^^ Let me look at that and come back to you
Edit: It was from here: https://github.com/mumble-voip/ice So not from the original zeroc-ice project, really sorry about that. With that information I guess you would prefer the ICE_LIBNAME way, right? I'll look into that and make a PR when I've got a solution. Thank you very much for your help so far.
Edit2: Possible PR solution https://github.com/dg0yt/vcpkg/pull/23
This is an automated message. Per our repo policy, stale issues get closed if there has been no activity in the past 180 days. The issue will be automatically closed in 14 days. If you wish to keep this issue open, please add a new comment.
not yet really fixed. Only when https://github.com/microsoft/vcpkg/pull/33648 or something similar gets merged
This is an automated message. Per our repo policy, stale issues get closed if there has been no activity in the past 180 days. The issue will be automatically closed in 14 days. If you wish to keep this issue open, please add a new comment.
Hello there,
I have a little problem with zeroc-ice:x64-windows-static-md, as cmake does not find the library ice37++11.lib. Which in some way is correct, because it does not exist, only ice++11.lib exists. With the x64-windows(shared) build the lib name is indeed ice37++11.lib.
So now my first question would be which one is the "correct" way for vcpkg library names? Because as far as I can see some packages have the versions in them, some do not. Do we expect a difference between static and shared builds?
Now I know where it stems from in the zeroc-ice build: If we use cmake to build zeroc-ice the shared and static build are both named the same: ice37++11.lib If we use vcxproj(used by the vcpkg port) the shared build is named ice37++11.lib and the static build ice++11.lib
~I could not track down where the naming scheme inside vcxproj stems from, is that perhaps a Microsoft standard and done by msbuild automatically without it being set anywhere explicitly?~ Edit: In the ice.cpp.props the
TargetName
property is set forDynamicLibrary
but not forStaticLibrary
, so we could introduce a patch file to output the static library with a version suffix as well. If that is not wanted by the vcpkg library naming, then where can I tell vcpkg that cmake should search for the library name without a version suffix?Thank you so much in advance for all the help and info you can give me