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

[libigl] Always install as header-only library #14888

Merged
merged 4 commits into from
Feb 9, 2021

Conversation

FabienPean
Copy link
Contributor

In #14376, triplet *-windows-dynamic version was updated to rely on the library as header-only while leaving the *-static triplet intact. However, the static library archive still suffers from the same reported issues: huge size and not all combination of explicitly instantiated templates exist. This PR forces installs of libigl as header-only library for both static and dynamic triplets. (Particularly useful for vcpkg on linux where the default triplet has static linkage by default)

Let me know if this PR is an agreeable change on your side.

@NancyLi1013 NancyLi1013 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Dec 2, 2020
@FabienPean FabienPean marked this pull request as ready for review December 2, 2020 11:42
@FabienPean
Copy link
Contributor Author

The PR shows failure for another port.

@JackBoosY
Copy link
Contributor

Maybe we should add an new feature header-only to this port instead of force it to be a header only port.

@FabienPean FabienPean force-pushed the fix/libigl_header_only branch from 870723f to d7184f8 Compare December 21, 2020 13:04
@FabienPean
Copy link
Contributor Author

Added header-only as a feature

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Dec 23, 2020
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

The "header-only" feature doesn't comply with our guidelines for features, namely, that they do not provide "alternative" selections. See https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#do-not-use-features-to-control-alternatives-in-published-interfaces

Crucially, any downstream consumer who is passing -ligl (or similar to link the static lib) would be broken by turning on this "feature".

It might be acceptable to make such a chance unconditional if the current users of the port think that makes sense.

/cc @patrikhuber @deGravity @xarthurx

@BillyONeal BillyONeal added requires:discussion and removed info:reviewed Pull Request changes follow basic guidelines labels Dec 23, 2020
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.

Since the comments in the upstream project indicate that it is a "header only library", I think the right way forward is to always just do that.

With this change, LGTM!

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Feb 5, 2021
@JackBoosY JackBoosY requested a review from BillyONeal February 7, 2021 06:50
@ras0219-msft ras0219-msft merged commit 7e01ac1 into microsoft:master Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants