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

[openimageio/libsquish] Export libsquish cmake target and fix find dependency libsquish #20240

Merged
merged 13 commits into from
Oct 11, 2021

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Sep 18, 2021

As @dg0yt said in #20092 (comment):

FindLibsquish.cmake must be patched to handle the debug import lib which is actually named squishd.lib and thus does not match squish:
https://github.com/OpenImageIO/oiio/blob/eb4b84e74d38db139bf07ed321a3d63523fa6464/src/cmake/modules/FindLibsquish.cmake#L21-L25

Fixes #20233.
Related: #20092.

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Sep 18, 2021
@JackBoosY
Copy link
Contributor Author

@HWiman-ICONIC Can you please test this PR?

Thanks.

@HWiman-ICONIC
Copy link

If I have understood the conversation in #20092 correctly, libsquish is optional, correct @lgritz? However, I get the following after this PR indicating that libsquish is required. Which is correct?

PS C:\Dev\vcpkg> .\vcpkg.exe install openimageio[webp]:x64-windows
Computing installation plan...
The following packages will be built and installed:

  • libsquish[core]:x64-windows -> 1.15#9
    openimageio[core,webp]:x64-windows -> 2.3.7.2#3

@HWiman-ICONIC
Copy link

Having installed and running the minimal sample supplied to issue #20233 I get:

CMake Error at C:/Dev/vcpkg/scripts/buildsystems/vcpkg.cmake:786 (_find_package):
1> [CMake] By not providing "FindLibsquish.cmake" in CMAKE_MODULE_PATH this project
1> [CMake] has asked CMake to find a package configuration file provided by
1> [CMake] "Libsquish", but CMake did not find one.
1> [CMake]
1> [CMake] Could not find a package configuration file provided by "Libsquish" with
1> [CMake] any of the following names:
1> [CMake]
1> [CMake] LibsquishConfig.cmake
1> [CMake] libsquish-config.cmake

@dg0yt
Copy link
Contributor

dg0yt commented Sep 18, 2021

If I have understood the conversation in #20092 correctly, libsquish is optional

AFAIU OpenImageIO will always use libsquish. If it doesn't find an external package, it will use an internal copy (aka vendored dependency). Package manager dislike vendored copies, for good reasons.

@HWiman-ICONIC
Copy link

If I have understood the conversation in #20092 correctly, libsquish is optional

AFAIU OpenImageIO will always use libsquish. If it doesn't find an external package, it will use an internal copy (aka vendored dependency). Package manager dislike vendored copies, for good reasons.

Ok thanks, I see. Then it makes sense to force dependency of libsquish in vcpkg of course.

@JackBoosY JackBoosY marked this pull request as ready for review September 22, 2021 02:41
@JackBoosY
Copy link
Contributor Author

@HWiman-ICONIC Can you please use this branch, rebuild libsquish first and try to build oiio again?

@HWiman-ICONIC
Copy link

@HWiman-ICONIC Can you please use this branch, rebuild libsquish first and try to build oiio again?

I can´t get this to work.
By not providing "FindLibsquish.cmake" in CMAKE_MODULE_PATH this project 1> [CMake] has asked CMake to find a package configuration file provided by 1> [CMake] "Libsquish", but CMake did not find one. 1> [CMake] 1> [CMake] Could not find a package configuration file provided by "Libsquish" with 1> [CMake] any of the following names: 1> [CMake] 1> [CMake] LibsquishConfig.cmake 1> [CMake] libsquish-config.cmake
What I did:
.\vcpkg.exe remove libsquish:x64-windows --recurse .\vcpkg.exe remove openimageio:x64-windows .\vcpkg.exe install libsquish:x64-windows .\vcpkg.exe openimageio libsquish:x64-windows
Went ok with results:
find_package(unofficial-squish CONFIG REQUIRED) target_link_libraries(main PRIVATE unofficial-squish::squish) and find_package(OpenImageIO CONFIG REQUIRED) target_link_libraries(main PRIVATE OpenImageIO::OpenImageIO OpenImageIO::OpenImageIO_Util)
Edited the CMakeLists accordingly

@JackBoosY
Copy link
Contributor Author

-- Found unofficial-squish  from CONFIG

@HWiman-ICONIC Did you clone https://github.com/JackBoosY/vcpkg.git and use my branch dev/jack/20233 to build?

@HWiman-ICONIC
Copy link

-- Found unofficial-squish  from CONFIG

@HWiman-ICONIC Did you clone https://github.com/JackBoosY/vcpkg.git and use my branch dev/jack/20233 to build?

Yes, I did. And now I have done it once again with complete rebuild of all dependencies and get the same result. What results do you get if you create a default CMake project in VS and edit the CMakeLists.txt file to? :

# project specific logic here.
#
cmake_minimum_required (VERSION 3.8)

find_package(unofficial-squish CONFIG REQUIRED)
find_package(OpenImageIO CONFIG REQUIRED)

# Add source to this project's executable.
add_executable (CMakeProject15 "CMakeProject15.cpp" "CMakeProject15.h")

# TODO: Add tests and install targets if needed.
target_link_libraries(CMakeProject15 
    PRIVATE
    unofficial-squish::squish
    OpenImageIO::OpenImageIO OpenImageIO::OpenImageIO_Util
        )```

I still get:
```1> [CMake]   By not providing "FindLibsquish.cmake" in CMAKE_MODULE_PATH this project
1> [CMake]   has asked CMake to find a package configuration file provided by
1> [CMake]   "Libsquish", but CMake did not find one.```

@JackBoosY
Copy link
Contributor Author

Depends on AcademySoftwareFoundation/OpenImageIO#3108.

@JackBoosY JackBoosY added the depends:upstream-changes Waiting on a change to the upstream project label Sep 26, 2021
@JackBoosY JackBoosY removed the depends:upstream-changes Waiting on a change to the upstream project label Oct 8, 2021
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 36fe65216518ac13d8cc06fafc6887599818156e -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/l-/libsquish.json b/versions/l-/libsquish.json
index 5554847..353bf55 100644
--- a/versions/l-/libsquish.json
+++ b/versions/l-/libsquish.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "191bc228fe3fe6bd8628aaa767abca093b772202",
+      "git-tree": "d5f07a631c6bfd0918337856979c707caa85b7b4",
       "version-string": "1.15",
       "port-version": 9
     },
diff --git a/versions/o-/openimageio.json b/versions/o-/openimageio.json
index a44385a..abeb261 100644
--- a/versions/o-/openimageio.json
+++ b/versions/o-/openimageio.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "a1a204687895abc9c041d97f8a2dd6e36448f2b2",
+      "git-tree": "7e3fcbce391548117f0ba9b08353ee4fd225506b",
       "version": "2.3.7.2",
       "port-version": 3
     },

@JackBoosY
Copy link
Contributor Author

@HWiman-ICONIC Ping for test again.

@HWiman-ICONIC
Copy link

@HWiman-ICONIC Ping for test again.

Confirmed, PR ready to be merged to master I guess.

Furthermore, OpenImageIO have now merged AcademySoftwareFoundation/OpenImageIO#3108 to OpenImageIO master.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 36fe65216518ac13d8cc06fafc6887599818156e -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/l-/libsquish.json b/versions/l-/libsquish.json
index 5554847..353bf55 100644
--- a/versions/l-/libsquish.json
+++ b/versions/l-/libsquish.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "191bc228fe3fe6bd8628aaa767abca093b772202",
+      "git-tree": "d5f07a631c6bfd0918337856979c707caa85b7b4",
       "version-string": "1.15",
       "port-version": 9
     },

@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Oct 11, 2021
@BillyONeal BillyONeal merged commit a6768f6 into microsoft:master Oct 11, 2021
@BillyONeal
Copy link
Member

Thanks :)

@JackBoosY JackBoosY deleted the dev/jack/20233 branch October 12, 2021 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OpenImageIO] [libsquish] Dependent squish library is not copied in debug mode
6 participants