vcpkg icon indicating copy to clipboard operation
vcpkg copied to clipboard

[embree3] Expose project options, plus add NEON ISAs as features

Open theblackunknown opened this issue 2 years ago • 7 comments

If this PR updates an existing port, please uncomment and fill out this checklist:

  • [x] Changes comply with the maintainer guide
  • [x] SHA512s are updated for each updated download
  • [x] The "supports" clause reflects platforms that may be fixed by this new version
  • [x] Any fixed CI baseline entries are removed from that file.
  • [x] Any patches that are no longer applied are deleted from the port's directory.
  • [x] The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • [x] Only one version is added to each modified port's versions file.

theblackunknown avatar Dec 30 '23 00:12 theblackunknown

Looking at existing PR/issue there may be some redundancy with #30548 Something to double check before removing the draft status

Also this PR follows the suggestion to avoid listing explicitly which ISA to target which may be a good thing

theblackunknown avatar Dec 30 '23 17:12 theblackunknown

All feature tested on the following triplets:

  • x64-windows
  • x64-windows-static
  • x86-windows

Adela0814 avatar Jan 04 '24 08:01 Adela0814

Tagged vcpkg-team-review to answer this question:

Today, we would refuse to merge features that try to change ISA like this, under the 'do not implement alternatives as features' rule. However, embree3 already has other ISA features, should we make an exception in that this adds one like the others? See also, last time the maintainers weighed in on this topic: https://github.com/microsoft/vcpkg/pull/29093#issuecomment-1465969155

Alternatively, should we remove all ISA features entirely, because they are broken?

BillyONeal avatar Jan 04 '24 19:01 BillyONeal

I'll vote for removal. Stuff like this can be set via VCPKG_CMAKE_COINFIGURE_OPTIONS or the port needs to inspect compiler flags to enable those.

Neumann-A avatar Jan 04 '24 20:01 Neumann-A

@BillyONeal @Neumann-A great discussion ! I took the liberty to implement an embree3 port where we get rid of ISA features and handle it as best we can in the portfile.

I gave it a quick test on an internal project which target both native and emscripten platforms. For Emscripten, I had to hijack the ISA selection because it was incorrect otherwise letting Embree select the default one by itself seems to work fine.

Let me know what you think of 6d4e95e8bc5e0223ecb1501e39eddca4ceb990c0

theblackunknown avatar Jan 05 '24 15:01 theblackunknown

Hey @BillyONeal @Neumann-A, did you have the time to look at my latest revision which should take into account to your feedbacks ?

theblackunknown avatar Jan 13 '24 20:01 theblackunknown

Gentle reminder, @BillyONeal @Neumann-A, did you have the time to look at my latest revision which should take into account to your feedbacks ?

theblackunknown avatar Feb 09 '24 21:02 theblackunknown

The title of the PR should be changed to reflect the new changes.

Indeed, I'll find something more appropriate before the end of the week.

theblackunknown avatar Mar 06 '24 22:03 theblackunknown