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

[opensubdiv] Upgrade to 3.4.4 #26628

Merged
merged 3 commits into from
Oct 9, 2022
Merged

[opensubdiv] Upgrade to 3.4.4 #26628

merged 3 commits into from
Oct 9, 2022

Conversation

Danielmelody
Copy link
Contributor

Describe the pull request

  • What does your PR fix?

None

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

    Nothing changed

  • 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/

@JonLiu1993 JonLiu1993 changed the title Upgrade opensubdiv to 3.4.4 [opensubdiv ] Upgrade to 3.4.4 Sep 1, 2022
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Sep 1, 2022
github-actions[bot]
github-actions bot previously approved these changes Sep 1, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/opensubdiv/vcpkg.json

Valid values for the license field can be found in the documentation

@Danielmelody
Copy link
Contributor Author

@JonLiu1993 just updated, please re-approve workflows

github-actions[bot]
github-actions bot previously approved these changes Sep 1, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/opensubdiv/vcpkg.json

Valid values for the license field can be found in the documentation

JonLiu1993
JonLiu1993 previously approved these changes Sep 2, 2022
github-actions[bot]
github-actions bot previously approved these changes Sep 2, 2022
@JonLiu1993
Copy link
Member

@Danielmelody, Thanks for your pr, when I tested the features by command ./vcpkg install opensubdiv[*]:x64-windows
I got this error:

F:\Feature-test\opensubdiv\vcpkg\buildtrees\opensubdiv\src\d6f41866d5-0c704570eb.clean\examples\common\glPtexMipmapTexture.cpp
F:\Feature-test\opensubdiv\vcpkg\buildtrees\opensubdiv\src\d6f41866d5-0c704570eb.clean\examples\common\glPtexMipmapTexture.cpp(25): fatal error C1083: Cannot open include file: 'glLoader.h': No such file or directory

could you please take a look?

@JonLiu1993
Copy link
Member

Ping @Danielmelody for response, Is work still being done for this PR?

@Danielmelody
Copy link
Contributor Author

@JonLiu1993 I have been busy working with original repo, now that they have merged my PR I will post here a patch later.

@Danielmelody
Copy link
Contributor Author

@JonLiu1993 Updated, please have a look.
I also combined the license commit into one.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for opensubdiv have changed but the version was not updated
version: 3.4.4
old SHA: 360249ebfc9a41a84fe9fe84ac436165cee04003
new SHA: 8a3aec2e545ebcd0e21e1cfe5c779f2bdc19b081
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@JonLiu1993
Copy link
Member

@Danielmelody , please take a look:

##[error]Found the following errors:
    Error: While reading versions for port opensubdiv from file: C:\a\1\s\versions\o-\opensubdiv.json
       File declares version `3.4.4` with SHA: 360249ebfc9a41a84fe9fe84ac436165cee04003
       But local port with the same version has a different SHA: 8a3aec2e545ebcd0e21e1cfe5c779f2bdc19b081
       Please update the port's version fields and then run:

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

@Danielmelody
Copy link
Contributor Author

@JonLiu1993 Fixed

github-actions[bot]
github-actions bot previously approved these changes Sep 13, 2022
@JonLiu1993
Copy link
Member

opensubdiv[*]:x64-windows tested successfully, but when I tested the features by command ./vcpkg install opensubdiv[*]:x64-windows-static
I got this error:

LINK : warning LNK4044: unrecognized option '/openmp'; ignored
   Creating library bin\glFVarViewer.lib and object bin\glFVarViewer.exp
LINK : warning LNK4098: defaultlib 'LIBCMT' conflicts with use of other libs; use /NODEFAULTLIB:library
OpenCL.lib(icd_windows_hkr.c.obj) : error LNK2019: unresolved external symbol __imp_CM_Get_Child referenced in function khrIcdOsVendorsEnumerateHKR
OpenCL.lib(icd_windows_hkr.c.obj) : error LNK2019: unresolved external symbol __imp_CM_Get_Device_IDW referenced in function khrIcdOsVendorsEnumerateHKR
OpenCL.lib(icd_windows_hkr.c.obj) : error LNK2019: unresolved external symbol __imp_CM_Get_Device_ID_ListW referenced in function khrIcdOsVendorsEnumerateHKR
OpenCL.lib(icd_windows_hkr.c.obj) : error LNK2019: unresolved external symbol __imp_CM_Get_Device_ID_List_SizeW referenced in function khrIcdOsVendorsEnumerateHKR
OpenCL.lib(icd_windows_hkr.c.obj) : error LNK2019: unresolved external symbol __imp_CM_Get_DevNode_PropertyW referenced in function khrIcdOsVendorsEnumerateHKR
OpenCL.lib(icd_windows_hkr.c.obj) : error LNK2019: unresolved external symbol __imp_CM_Get_DevNode_Status referenced in function ProbeDevice
OpenCL.lib(icd_windows_hkr.c.obj) : error LNK2019: unresolved external symbol __imp_CM_Get_Sibling referenced in function khrIcdOsVendorsEnumerateHKR
OpenCL.lib(icd_windows_hkr.c.obj) : error LNK2019: unresolved external symbol __imp_CM_Locate_DevNodeW referenced in function khrIcdOsVendorsEnumerateHKR
OpenCL.lib(icd_windows_hkr.c.obj) : error LNK2019: unresolved external symbol __imp_CM_Open_DevNode_Key referenced in function ReadOpenCLKey
bin\glFVarViewer.exe : fatal error LNK1120: 9 unresolved externals

@JonLiu1993
Copy link
Member

@Danielmelody , Please fix the conflicts.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for opensubdiv have changed but the version was not updated
version: 3.4.4
old SHA: 3cfd1e3359700b453df9c4c2e05fc85c944786f7
new SHA: 246a4cf53b7d0d15391f0e479ea5c1ad4fc320a2
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@Danielmelody
Copy link
Contributor Author

@JonLiu1993 rebased

@JonLiu1993 JonLiu1993 added requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function and removed requires:author-response labels Sep 28, 2022
@JonLiu1993
Copy link
Member

@Danielmelody, when I tested the features by command ./vcpkg install opensubdiv[*]:x64-windows
I got this error:

F:\Feature-test\opensubdiv\vcpkg\buildtrees\opensubdiv\src\d6f41866d5-0c704570eb.clean\opensubdiv\osd\tbbEvaluator.cpp(28): fatal error C1083: Cannot open include file: 'tbb/task_scheduler_init.h': No such file or directory

@Danielmelody
Copy link
Contributor Author

@JonLiu1993 while trying to resolve this issue, I find it is because of tbb upgrading version. I try to pin old tbb with overrides but not work.
opensubdiv/vcpkg.json

{
  "name": "opensubdiv",
  "version-semver": "3.4.4",
  "description": "An Open-Source subdivision surface library.",
  "homepage": "https://github.com/PixarAnimationStudios/OpenSubdiv",
  "license": "Apache-2.0",
  "supports": "!arm & !uwp",
  "dependencies": [
    {
      "name": "vcpkg-cmake",
      "host": true
    }
  ],
  "features": {
    "cuda": {
      "description": "Enable CUDA backend",
      "dependencies": [
        "cuda"
      ]
    },
    "dx": {
      "description": "Enable DirectX support",
      "dependencies": [
        {
          "name": "directxsdk",
          "features": [
            "xp"
          ]
        }
      ]
    },
    "glew": {
      "description": "Path to glew",
      "dependencies": [
        "glew"
      ]
    },
    "glfw": {
      "description": "Path to glfw",
      "dependencies": [
        "glfw3"
      ]
    },
    "omp": {
      "description": "Enable OpenMP backend"
    },
    "opencl": {
      "description": "Enable OpenCL backend",
      "dependencies": [
        "opencl"
      ]
    },
    "opengl": {
      "description": "Enable OpenGL",
      "dependencies": [
        "opengl"
      ]
    },
    "ptex": {
      "description": "Path to Ptex",
      "dependencies": [
        "ptex"
      ]
    },
    "tbb": {
      "description": "Enable TBB backend",
      "dependencies": [
        "tbb"
      ]
    },
    "true-deriv-eval": {
      "description": "Enable true derivative evaluation for Gregory basis patches"
    }
  },
  "overrides": [
    {
      "name": "tbb", "version-string": "2020_U3", "port-version": 8
    }
  ]
}

@Danielmelody
Copy link
Contributor Author

There are 2 things I think not working as expect.

  1. While upgrading tbb, all ports that depends on it just simply broken. I 've test install opensubdiv on a new clone and I get the same error.
  2. The override feature seems not functional, too.

@Danielmelody
Copy link
Contributor Author

Danielmelody commented Sep 28, 2022

It should not be tbb's maintainer's nor opensubdiv's maintainer's responsibility to resolve those problem on their upgrading PR. Vcpkg should have some mechanism for sparse locking version upperbound (eg. allow patch version upgrade but lock minor version), in case dependent port's update breaks current port unexpectlly.

@Danielmelody
Copy link
Contributor Author

@JonLiu1993 @JackBoosY
You have mentioned that the vfx industry does not officially support TBB 2021
PixarAnimationStudios/OpenUSD#1471
While in this PR
#26284
It just leave the usd port incompatible with baseline's tbb.
If #26284 could be merged why this PR is blocked?

@Danielmelody Danielmelody changed the title [opensubdiv ] Upgrade to 3.4.4 [opensubdiv] Upgrade to 3.4.4 Oct 6, 2022
@JonLiu1993
Copy link
Member

@JonLiu1993 @JackBoosY You have mentioned that the vfx industry does not officially support TBB 2021 PixarAnimationStudios/USD#1471 While in this PR #26284 It just leave the usd port incompatible with baseline's tbb. If #26284 could be merged why this PR is blocked?

I think there is no problem with the version upgrade of opensubdiv involved in this pr, so I agree to merge this pr, but I will test other features except tbb

@JonLiu1993
Copy link
Member

Feature cuda,dx,glew,glfw,omp,ptex,true-deriv-eval tested successfully in the following triplet:

  • x64-windows
  • x64-windows-static

@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function labels Oct 8, 2022
@vicroms vicroms merged commit 09d33fe into microsoft:master Oct 9, 2022
@vicroms
Copy link
Member

vicroms commented Oct 9, 2022

It should not be tbb's maintainer's nor opensubdiv's maintainer's responsibility to resolve those problem on their upgrading PR. Vcpkg should have some mechanism for sparse locking version upperbound (eg. allow patch version upgrade but lock minor version), in case dependent port's update breaks current port unexpectlly.

Allowing version restrictions like those in the main catalog would make everything untestable soon enough. What if a library wants foo = 2.0.* and another one wants foo >= 3.0.0? Do we run CI twice to satisfy both? And as the number of conflicts increases, the combinatorial explosion becomes unmanageable.

While we acknowledge that updating foundational libraries like tbb may break some downstreams we believe that it is the right choice to not hold the universe back.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 9, 2022

Allowing version restrictions like those in the main catalog would make everything untestable soon enough.

True. But the current approach might make everything unmaintainable.

What if a library wants foo = 2.0.* and another one wants foo >= 3.0.0?

Moving the burden of patching all downstream ports to the maintainer of port foo doesn't seem to scale much better. He/she will have to do with ports which haven't been updated in years, or which use unsual build systems. And we won't even look at triplets which are not tested in CI.

And the problem is real:

  • The openexr update from 2 to 3 needed a lot of patches to downstream ports.
  • The gtk update from 3 to 4 broke wxwidgets for linux. We had to repackage gtk3. This took ages because the re-adding had to meet today's requirements for port quality which in turn needed adjustments to CI images.

Do we run CI twice to satisfy both?

Even now, we don't run CI enough. Some conflicts are simply hidden by skip entries in CI baseline. Some conflicts are hidden behind optional features.

  • openimage[opencolorio] became temporarily unusable because it was not tested in CI when opencolorio was updated to use port imath 3 while openimageio was still using imath (2) coming with openexr 2.

@Danielmelody
Copy link
Contributor Author

Danielmelody commented Oct 9, 2022

The combinatorial explosion does not come from version upper bound, but from all version range requirement. Since we have lower bound constraint, introducing upper bound constraint is reasonable.

What if a library wants foo = 2.0.* and another one wants foo >= 3.0.0

In this case, the constraint just resolve failed, this situation is just like a lib specify foo= 2.0.0 (by overrides), and b wants foo >= 3.0.0. The only difference would be a patch upper bound allows upgrading patch while overrides pin a exact version.

@Danielmelody
Copy link
Contributor Author

The upper bound requirement of libraries, on the other hand, should be avoid whenever possible. But if can't, it is necessary. CI can perform test for all downstream ports of an upstream port update, and asking PR author to only add upper bound for downstream port that is broken.
The removal of this upper bound should be done by downstream port maintainers, which is a natural way for such a package manager.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants