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

dox: Fix vcpkg install everything command #368

Closed
wants to merge 1 commit into from

Conversation

Squareys
Copy link
Contributor

Hi @mosra !

Just came by this and remembered a discussion on gitter. To ensure it's still bareable for vcpkg users to install everything on windows I went trough all available packages and listed them.

Otoh: This is temporary and unacceptable in my opinion in terms of usability, since the average user will try vcpkg install magnum[*] first and having to check the docs to copy the entire list is not great either.

I propose vcpkg install magnum[windowlesscontext] to autodetect which one to build or (if some certain platforms have multiple contexts available) automatically detect features that cannot be built and silently (or with warning "skipping") remove them from the list.

I cannot promise that I will get to implementing such a thing too soon, though. Maybe with the next release update of the ports.

Cheers, Jonathan

@mosra mosra added this to the 2019.0b milestone Aug 24, 2019
@mosra
Copy link
Owner

mosra commented Aug 24, 2019

The real solution for this would be Vcpkg having per-platform Default-Features. I asked for this when Linux support for the Magnum vcpkg package was added but didn't get any answer. Maybe opening a feature request for this? Or, optimistically, maybe such a feature was already added since then?

or (if some certain platforms have multiple contexts available) automatically detect features that cannot be built and silently (or with warning "skipping") remove them from the list.

That would be an alternative solution -- can presence of * be detected? If so, then it could just prune the unavailable features ... but then vcpkg would think those are installed, while they in fact aren't?

@codecov-io
Copy link

codecov-io commented Aug 24, 2019

Codecov Report

Merging #368 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #368   +/-   ##
=======================================
  Coverage   71.21%   71.21%           
=======================================
  Files         348      348           
  Lines       18012    18012           
=======================================
  Hits        12828    12828           
  Misses       5184     5184

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acf06eb...9fe1ccd. Read the comment docs.

@mosra
Copy link
Owner

mosra commented Aug 25, 2019

Um, before I merge this -- didn't vcpkg install package[bla] also implicitly include all defaults (and saying vcpkg install package[core,bla] didn't)? I remember some such thing. If that's the case, the line could be much shorter.

@ras0219-msft
Copy link

We do have adding qualifier support to default features on our backlog, however we've identified a workaround in the meantime: Essentially, add a feature which (itself) represents the defaults, have that be the only default, and then that feature can use qualifiers on further dependencies.

Example: https://github.com/microsoft/vcpkg/blob/master/ports/cpprestsdk/CONTROL

@cbezault
Copy link

Yes that's the case. package[bla] will include default features while package[core,bla] won't.

@mosra
Copy link
Owner

mosra commented Aug 26, 2019

@ras0219-msft oh, wonderful -- didn't know the (windows) / (!uwp) qualifiers were a thing until now. That solves everything (and it's no problem that it's a slight workaround), thank you!

@alanjfs
Copy link
Contributor

alanjfs commented Sep 28, 2019

Ooo, just spotted this. This would have helped me the other day. Related to #373.

@Squareys
Copy link
Contributor Author

So, summing up the current state of this PR:

  • This changes the docs to recommend avoiding magnum[*] as it will install features that are platform-dependent. There is no native way to declare a feature to be available only on a single platform.
  • Vcpkg seem to be installing magnum[*] for tests, which causes problems out of aforementioned reason.
  • Platform dependent defaults are possible with the mentioned workaround.
  • Since the qualifiers can be used to have different dependencies for different platforms, this could be used to replace windowlesswglapplication and windowlessglxapplication with windowlessapplication, which then could be used to set the correct platform-dependent cmake option.

Anything I forgot? I think the most userfriendly would be to support vcpkg install magnum[*] in a way, and it would fix the tests that they do at vcpkg.

@mosra
Copy link
Owner

mosra commented Feb 25, 2020

This is unfortunately problematic because there are features that are inevitably available just on a single platform, even more so with the introduction of filesystems (mosra/corrade#39). Collapsing the current problematic ones to just glcontext, windowlessapplication could "fix" things right now but there will be still issues where for example on Linux you can have both GLX or EGL installed, or just one, or neither, and the package has no way to check for that.

One idea that could fix [*] is that the portfile just ignores (or ignores with a warning) features that are definitely not available on a particular platform (so e.g. it won't add -DWITH_GLXCONTEXT=ON to CMake on Windows). Could that work?

@mosra
Copy link
Owner

mosra commented Jul 2, 2020

This is now fixed with microsoft/vcpkg#12211 by ignoring features that are incompatible with given platform.

@alanjfs
Copy link
Contributor

alanjfs commented Jul 2, 2020

Oh joy, just in time for my return to Magnum. :D Thanks @mosra, see you in the chat shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants