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

[assimp] Fix generated -config files to use more generic form #13264

Merged
merged 7 commits into from
Sep 10, 2020

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Sep 1, 2020

Assimp has two sets of config files: one that properly handles dependencies and one that doesn't. This PR switches which set to install.

Additionally, this PR causes the embedded IrrXML source to be built directly into the assimp library instead of as a separate binary for simpler consumption.

@BillyONeal
Copy link
Member

How does this compare to #12437 ?

@NancyLi1013 NancyLi1013 self-assigned this Sep 1, 2020
@BillyONeal
Copy link
Member

OK, this is fine to me but please sync with @JackBoosY on the other review.

@NancyLi1013
Copy link
Contributor

The failures on x64-linux due to this:

CMake Error at scripts/cmake/vcpkg_fixup_cmake_targets.cmake:72 (message):
  '/mnt/vcpkg-ci/packages/assimp_x64-linux/debug/lib/cmake/assimp' does not
  exist.
Call Stack (most recent call first):
  ports/assimp/portfile.cmake:37 (vcpkg_fixup_cmake_targets)
  scripts/ports.cmake:79 (include)

@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 2, 2020
@petersteneteg
Copy link
Contributor

Assimps own hardcoded config files uses the target name "assimp::assimp" but with this change the name will be "Assimp::assimp" which breaks compatibility...

@ras0219
Copy link
Contributor Author

ras0219 commented Sep 2, 2020

I've added in a backwards-compatible alias that forwards assimp::assimp to Assimp::assimp. It's unfortunate that their cmake files have two different target names, but that's life I suppose.

This PR should now handle all cases covered in #12437. @JackBoosY Are there any remaining uncovered issues as far as you can tell?

@cenit
Copy link
Contributor

cenit commented Sep 2, 2020

In #12199 I had to modify assimp a lot, involving some of the things here

@JackBoosY
Copy link
Contributor

Will test usage on Linux.

@JackBoosY
Copy link
Contributor

Passed.

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

May I ask you a comment about the fixes in #12199? IMHO those are very different and maybe more complete. These fixes here do not solve almost anything related to the problems I had there in using assimp

@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

Also another comment about the note in the portfile: irrlicht contains irrxml, but technically that port should use “external” irrxml (which I added and worked in the above PR)

@Neumann-A
Copy link
Contributor

hmmm this also broke #13464. So why is irrxml embedded and not an extra port?

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.
Projects
None yet
8 participants