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

[magnum/magnum-plugins] fix magnum-plugins building all magnum dependencies #16657

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

fran6co
Copy link
Contributor

@fran6co fran6co commented Mar 11, 2021

Describe the pull request

Fixes error:

  By not providing "FindAssimp.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "Assimp", but
  CMake did not find one.
  Could not find a package configuration file provided by "Assimp" with any
  of the following names:
    AssimpConfig.cmake
    assimp-config.cmake
  Add the installation prefix of "Assimp" to CMAKE_PREFIX_PATH or set
  "Assimp_DIR" to a directory containing one of the above files.  If "Assimp"
  provides a separate development package or SDK, be sure it has been
  installed.

The assimp library in vcpkg is all lower case and Magnum expects it camel case.

Also installing magnum-plugins triggers the installation of all the features in magnum.

@PhoebeHui PhoebeHui self-assigned this Mar 12, 2021
@PhoebeHui
Copy link
Contributor

@fran6co, thanks for the PR! could you update the baseline version?

Error: While reading versions for port magnum-plugins from file: C:\a\1\s\versions\m-\magnum-plugins.json
       Version `2020.06#4` was not found in versions file.
       Run:

           vcpkg x-add-version magnum-plugins

       to add the new port version.
To attempt to resolve all errors at once, run:

    vcpkg x-add-version --all

@PhoebeHui PhoebeHui added category:port-bug The issue is with a library, which is something the port should already support requires:author-response labels Mar 12, 2021
@fran6co
Copy link
Contributor Author

fran6co commented Mar 12, 2021

@PhoebeHui done

Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

Thanks for your updates!

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Mar 15, 2021
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This LGTM. +@Squareys +@mosra to possibly review this, otherwise I'll merge tomorrow.

@mosra
Copy link

mosra commented Mar 15, 2021

The assimp library in vcpkg is all lower case and Magnum expects it camel case.

We don't, we deliberately use Assimp instead of assimp and provide our own FindAssimp.cmake in order to fix various issues with Assimp's own CMake files, incomplete packaging in Homebrew and elsewhere... This change will only cause additional headaches on our side due to potential patch conflicts. I think the root cause is something else.

@fran6co may I ask where did you get this error? Was it during the package build or when using the magnum-plugins vcpkg package in your project?


(@ras0219-msft thanks a lot for the ping, is there some automated way to have @Squareys and me pinged on magnum package updates? It could save us a lot of time. Maybe through some CODEOWNERS file?)

@PhoebeHui PhoebeHui added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Mar 16, 2021
@fran6co
Copy link
Contributor Author

fran6co commented Mar 16, 2021

@mosra it happens when I'm using magnum-plugins. When I add the project with this:

find_package(MagnumPlugins CONFIG REQUIRED AssimpImporter)
target_link_libraries(app PRIVATE MagnumPlugins::AssimpImporter)

I get this error:

  Target "app" links to target "Assimp::Assimp" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?

If I add the alias of assimp::assimp to Assimp::Assimp I get the error I mentioned before:

 By not providing "FindAssimp.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "Assimp", but
  CMake did not find one.
  Could not find a package configuration file provided by "Assimp" with any
  of the following names:
    AssimpConfig.cmake
    assimp-config.cmake
  Add the installation prefix of "Assimp" to CMAKE_PREFIX_PATH or set
  "Assimp_DIR" to a directory containing one of the above files.  If "Assimp"
  provides a separate development package or SDK, be sure it has been
  installed.

@fran6co
Copy link
Contributor Author

fran6co commented Mar 16, 2021

Maybe the proper solution would be adding to the installation MagnumPlugins's FindAssimp.cmake as it's not being copied as part of the package

@mosra
Copy link

mosra commented Mar 16, 2021

Ah, I remember an older issue opened by you as well, coincidentally: mosra/magnum#436 For the reasons stated there, the solution (until I get this finally fixed) is to copy FindAssimp.cmake to your project module directory :)

The same behavior is with most other 3rd party dependencies in Magnum, so I don't think doing a patch inside vcpkg makes sense.

@fran6co
Copy link
Contributor Author

fran6co commented Mar 16, 2021

Ah, forgot about that... Well, I guess I can do the same, I'll remove the assimp change but keep the [core] change to avoid installing all magnum dependencies

@fran6co fran6co changed the title [magnum/magnum-plugins] fix assimp [magnum/magnum-plugins] fix magnum-plugins building all magnum dependencies Mar 16, 2021
@mosra
Copy link

mosra commented Mar 16, 2021

keep the [core] change to avoid installing all magnum dependencies

Missed this one completely. Good idea 👍

I'll ping you once mosra/magnum#436 gets fixed properly in Magnum itself :)

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Mar 16, 2021
@ras0219-msft
Copy link
Contributor

I think CODEOWNERS won't work due to write permissions, but we do have a maintainers field: https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/manifest-files.md#maintainers

@ras0219-msft ras0219-msft merged commit 37df512 into microsoft:master Mar 16, 2021
@ras0219-msft
Copy link
Contributor

LGTM, thanks everyone!

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:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants