[mdl] Add port MDL-SDK 2021.1.2

[theblackunknown]

Describe the pull request

• What does your PR fix?

Add a new port for MDL-SDK

• Which triplets are supported/not supported? Have you updated the CI baseline?

not all triplets supported, I have set up supports accordingly.

• Does your PR follow the maintainer guide?

Yes

• If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

[theblackunknown]

@JackBoosY @dg0yt I think I implemented the right split between my port mdl and its required build tool vcpkg-tool-clang7. I successfully tested it locally however I am struggling to get a meaningful output from the CI, moreover the build time seems to be pretty high those days. May I ask that you can start reviewing latest changes, and ideally give it a try to build the port ?

In the meantime, I'll wait for the last CI to finish and see if I can troubleshoot any errors there.

[Cheney-W]

Could you please handle the conflicts? Thank you!

[Cheney-W]

Pinging @theblackunknown for response. Is work still being done for this PR?

[theblackunknown]

Pinging @theblackunknown for response. Is work still being done for this PR?

Hi @Cheney-W , sorry for the delay. Yes this PR is still in progress, I have not found enough bandwidth to apply the change you requested but I'll try to do it next week.

[FrankXie05]

All features are tested successfully in the following triplet:

• x64-windows

[theblackunknown]

I'm tagging this PR for review with the whole team in our next Thursday meeting (7/7/22).

We usually don't accept PRs trying to set the compiler used for the port since compiler is a user setting. But this case is a different since it uses LLVM as a dependency rather than as the compiler; and upstream has listed Clang 7 as a hard-dependency.

Hi @vicroms and thanks for your interest in this PR !

Let me know after your team review if there is anything more I can do to help. I am also in touch with the upstream repository maintainers if that helps to change things there instead of patching the port.

[Cheney-W]

versions/m-/mdl.json(2,): error : While reading versions for port mdl from file: version/m-/mdl.json File declares version 2021.1.2 with SHA: f63c610c21422d2b345b1cf88f6df7b3e1cd1c20 But local port with the same version has a different SHA: 69e720adce8a980c2b7f6d7608f3d1466cd991b9 Please update the port's version fields and then run:

vcpkg x-add-version mdl
git add versions
git commit -m "Update version database"

to add a new version.

[theblackunknown]

versions/m-/mdl.json(2,): error : While reading versions for port mdl from file: version/m-/mdl.json File declares version 2021.1.2 with SHA: f63c610c21422d2b345b1cf88f6df7b3e1cd1c20 But local port with the same version has a different SHA: 69e720adce8a980c2b7f6d7608f3d1466cd991b9 Please update the port's version fields and then run:

vcpkg x-add-version mdl
git add versions
git commit -m "Update version database"

to add a new version.

Thanks for the reminder @Cheney-W , I am a bit swamped this week but I'll find time to put the final touch on this PR.

[FrankXie05]

All features are tested successfully in the following triplet:

• x64-windows

[JackBoosY]

Ping @vicroms for review again.

[theblackunknown]

Thanks for you review @Thomas1664 , I have integrated your suggestions.

[theblackunknown]

Is there anything else preventing this PR to be merged ?

[BillyONeal]

@ras0219-msft, @vicroms, @dan-shaw, @JavierMatosD, and I talked about this today and have the following additional statements:

1. Can a user meaningfully use this without a copy of LLVM 7 themselves? (e.g. should this LLVM 7.x come from the CUDA toolset itself?) We are assuming for purposes of this reply that the answer is no, if it is yes please come back because we need to rediscuss.
2. We are OK with this, and are explicitly saying it isn't like https://github.com/microsoft/vcpkg/pull/23827 , because the downloaded toolchain is not actually used to produce any of the resulting binaries; the toolchain file, flags, etc. the user supplies in their triplet is respected.
3. This situation needs a big comment in the portfile to explain the situation.
4. We would like the name to be mdl-sdk for consistency with upstream and https://repology.org/project/mdl-sdk/

We will not bring up the LLVM concern again (as long as the answer to 1 above remains no) and will be happy to merge this once a comment and the outstanding code review nitpicks are resolved.

[theblackunknown]

Thanks for the detailed review and very good questions @BillyONeal
I am away this week and probably couple of next ones after so there will be either no or slow progress during this time, I'll be back at the beginning of September to answer and tackle the remaining issues.

[BillyONeal]

• find_package(Threads REQUIRED QUITE) This is just bogus? NVIDIA/MDL-SDK#8

Upstream merged my fix :). But not in a release yet...

[theblackunknown]

• find_package(Threads REQUIRED QUITE) This is just bogus? NVIDIA/MDL-SDK#8

Upstream merged my fix :). But not in a release yet...

I have seen it, well done ! They will probably have it in the next minor release.

[theblackunknown]

4. We would like the name to be mdl-sdk for consistency with upstream and https://repology.org/project/mdl-sdk/

Good observation, I'll incorporate this change in my next change set.

[theblackunknown]

Check that the generated usage is accurate. mdl provides CMake targets: # this is heuristically generated, and may not be correct find_package(unofficial-mdl CONFIG REQUIRED) target_link_libraries(main PRIVATE unofficial::mdl::mdl_sdk unofficial::mdl::mdl_core) Can you confirm that this is how one is to use that?

To be fully correct, we should one or the other but noy both, I'll craft a dedicated usage file to clarify this. If you want to see it in action, here is a work in progress repo of mine to test this port (including proper bitcode generation) : https://github.com/theblackunknown-experiments/test-vcpkg-mdl

[theblackunknown]

3. This situation needs a big comment in the portfile to explain the situation.

I have added a quite detaillled notice on the use of Clang 7 for this port, let me know your thoughts on it.

[theblackunknown]

find_package(Boost QUIET) followed by an if that effectively makes it REQUIRED find_package(Boost QUIET) ditto turned into fatal error right after

Looking at the following if, I believe this is because they want to provide contextual information on the failure using __TARGET_ADD_DEPENDENCY_DEPENDS and __TARGET_ADD_DEPENDENCY_TARGET in their error message.

find_package(OpenGL QUIET) in vcpkg.json find_package(GLEW QUIET) Missing! I think we should add that as an explicit dependency in vcpkg.json? find_package(glfw3 QUIET) Ditto missing!

I believe those are only used to build examples and hence are never processed in this port.

find_package (Python COMPONENTS Interpreter Development) Seems to be missing?

Good catch yes, it is probably missing I'll double-check. However I see the same pattern as other find_package where they manually handle a FATAL_ERROR message afterwards if not found.

find_package(Qt5 COMPONENTS Core HINTS ${Qt5_DIR}) Missing, can you confirm this is shut off by -DMDL_ENABLE_QT_EXAMPLES:BOOL=OFF?

Yes it is: if MDL_ENABLE_QT_EXAMPLES is ever ON when this file is processed we end up with a FATAL_ERROR.

find_package(LibXml2) Missing! find_package(Backtrace) Missing! find_package(OCaml) Missing! Can we add -DLLVM_ENABLE_BINDINGS=OFF or will that damage the port? find_package(Subversion) Missing! But probably unused?

Those are inside the vendored LLVM , shoud I bother to add REQUIRED there ? Note that LLVM_ENABLE_BINDINGS is set to off in src\mdl\jit\llvm\CMakeLists.txt before processing the vendored version, so it is effectively disabled.

[BillyONeal]

Looking at the following if, I believe this is because they want to provide contextual information on the failure using __TARGET_ADD_DEPENDENCY_DEPENDS and __TARGET_ADD_DEPENDENCY_TARGET in their error message.

Yes, sorry, I wasn't noting things that need to be messed with, just that I had reviewed that and found it to be OK.

I believe those are only used to build examples and hence are never processed in this port.

Can you confirm by shutting them down with CMAKE_DISABLE_FIND_PACKAGE? https://cmake.org/cmake/help/latest/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html

Those are inside the vendored LLVM , shoud I bother to add REQUIRED there ?

The reason for requiring REQUIRED is to make it so the port doesn't build something different when different dependencies happen to be installed. That is:

vcpkg install libxml2
vcpkg install mdl-sdk

and

vcpkg install mdl-sdk
vcpkg install libxml2

need to give you the 'same' answer. As a result, this problem can be a problem even if it's coming from the vendored dependency. (Indeed, this is one of the reasons we discourage vendoring stuff so much: it is now 'your' problem)

CMAKE_DISABLE_FIND_PACKAGE can be used here too if you want.

[theblackunknown]

Using CMAKE_DISABLE_FIND_PACKAGE_<PackageName> to disable all optional packages you listed, the only hit I got is find_package(Backtrace) I do not know a lot but it looks like a system lib and hence we may not want to disable it ? or can it be provided by a port ? In anycase, this does not look like a requirement to build mdl-sdk port.

vcpkg install excerpt

CMake Warning at installed/arm64-osx/share/vcpkg-cmake/vcpkg_cmake_configure.cmake:302 (message):
  The following variables are not used in CMakeLists.txt:

      CMAKE_DISABLE_FIND_PACKAGE_GLEW
      CMAKE_DISABLE_FIND_PACKAGE_LibXml2
      CMAKE_DISABLE_FIND_PACKAGE_OCaml
      CMAKE_DISABLE_FIND_PACKAGE_OpenGL
      CMAKE_DISABLE_FIND_PACKAGE_Qt5
      CMAKE_DISABLE_FIND_PACKAGE_Subversion
      CMAKE_DISABLE_FIND_PACKAGE_glfw3

Last but not least, is it worth it to update the portfile with all those CMAKE_DISABLE_FIND_PACKAGE_<PackageName> anyway ?

[Cheney-W]

@BillyONeal Could you please take a look this PR again? Thank you!

[theblackunknown]

@BillyONeal can ask you to have another look when you get a chance please?

[BillyONeal]

Sorry it's taken a long time to reply to this; I've been under a whole bunch of guns for the artifacts feature and haven't been able to respond much.

Using CMAKE_DISABLE_FIND_PACKAGE_<PackageName> to disable all optional packages you listed, the only hit I got is find_package(Backtrace) I do not know a lot but it looks like a system lib and hence we may not want to disable it ? or can it be provided by a port ? In anycase, this does not look like a requirement to build mdl-sdk port.

vcpkg install excerpt

CMake Warning at installed/arm64-osx/share/vcpkg-cmake/vcpkg_cmake_configure.cmake:302 (message):
  The following variables are not used in CMakeLists.txt:

      CMAKE_DISABLE_FIND_PACKAGE_GLEW
      CMAKE_DISABLE_FIND_PACKAGE_LibXml2
      CMAKE_DISABLE_FIND_PACKAGE_OCaml
      CMAKE_DISABLE_FIND_PACKAGE_OpenGL
      CMAKE_DISABLE_FIND_PACKAGE_Qt5
      CMAKE_DISABLE_FIND_PACKAGE_Subversion
      CMAKE_DISABLE_FIND_PACKAGE_glfw3

Last but not least, is it worth it to update the portfile with all those CMAKE_DISABLE_FIND_PACKAGE_<PackageName> anyway ?

Perhaps what I noticed with grep isn't active unless some other conditions are met; I still would feel better about including them, even if they need to be tagged with maybe_unused. We have a long history of merging megaprojects like this and then kicking ourselves for several months afterward as optional dependencies cause "random failures" in CI because the install order differs, or that sort of thing.

[theblackunknown]

Sorry it's taken a long time to reply to this; I've been under a whole bunch of guns for the artifacts feature and haven't been able to respond much.

No worry about this @BillyONeal , I must say that I am very eager to play with vcpkg tool and the artifact feature !

I have added the CMAKE_DISABLE_FIND_PACKAGE you requested.

[theblackunknown]

Thank you very much to all reviewers for your help and patience on this one !