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

[field3d] Remove port #22463

Merged
merged 4 commits into from
Feb 10, 2022
Merged

[field3d] Remove port #22463

merged 4 commits into from
Feb 10, 2022

Conversation

chausner
Copy link
Contributor

Describe the pull request

  • What does your PR fix?

    This removes port field3d since it is no longer maintained upstream, cannot be updated and prevents the update of other ports. See the discussion at [field3d] Adapt port to use imath dependency #21079 for more details. In addition to removing the port, this PR removes the field3d feature from openimageio which depends on the removed port.

  • Does your PR follow the maintainer guide?

    Yes

Also removes the field3d feature from openimageio
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: Local changes detected for openimageio but no changes to version or port version.
-- Version: 2.3.10.1
-- Old SHA: f5a6955a1595a5d3ea429059d9be4ddedc8e7cab
-- New SHA: dd0525889e0a4e071226a18630dc9c23569a2c00
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 964e75806b4421814552facffef3d278ed9ef594 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 2b45cb5..1ee094d 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -2160,6 +2160,10 @@
       "baseline": "2019-12-19",
       "port-version": 1
     },
+    "field3d": {
+      "baseline": "1.7.3",
+      "port-version": 2
+    },
     "fixed-string": {
       "baseline": "0.1.0",
       "port-version": 1

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!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 964e75806b4421814552facffef3d278ed9ef594 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 5f62311..89b6cb8 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -2160,6 +2160,10 @@
       "baseline": "2019-12-19",
       "port-version": 1
     },
+    "field3d": {
+      "baseline": "1.7.3",
+      "port-version": 2
+    },
     "fixed-string": {
       "baseline": "0.1.0",
       "port-version": 1
diff --git a/versions/o-/openimageio.json b/versions/o-/openimageio.json
index c24a91a..6dbbf5f 100644
--- a/versions/o-/openimageio.json
+++ b/versions/o-/openimageio.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "dd0525889e0a4e071226a18630dc9c23569a2c00",
+      "git-tree": "0e18df7c572cf4be18ae2f86584dd1782a8db174",
       "version": "2.3.10.1",
       "port-version": 1
     },

@Hoikas
Copy link
Contributor

Hoikas commented Jan 10, 2022

If the port is simply unmaintained but not broken, deleting the port entirely seems inappropriate, IMO.

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!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 964e75806b4421814552facffef3d278ed9ef594 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 5f62311..89b6cb8 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -2160,6 +2160,10 @@
       "baseline": "2019-12-19",
       "port-version": 1
     },
+    "field3d": {
+      "baseline": "1.7.3",
+      "port-version": 2
+    },
     "fixed-string": {
       "baseline": "0.1.0",
       "port-version": 1

@JackBoosY
Copy link
Contributor

But we can keep the current version and add a message to notify the user that field3d will no longer update, right?

@chausner
Copy link
Contributor Author

chausner commented Jan 11, 2022

If the port is simply unmaintained but not broken, deleting the port entirely seems inappropriate, IMO.

The problem is that this port depends on openexr which many people would like to see updated. My first suggestion was to patch field3d to make it work with the new openexr version but this change was not accepted due to other reasons, see #21079. So in the end, the consensus in that discussion was that removing field3d would be the best workaround.

@JackBoosY
Copy link
Contributor

@chausner I think we don't need to remove this port, just remove this dependency relationship when the next openexr update would be okay.

@chausner
Copy link
Contributor Author

chausner commented Jan 11, 2022

@chausner I think we don't need to remove this port, just remove this dependency relationship when the next openexr update would be okay.

Can you explain what you suggest in more detail? openexr (or more precisely, the imath library that was still bundled with openexr 2.x) is a required dependency of field3d. It's not possible to remove it without breaking compilation of field3d.

@JackBoosY
Copy link
Contributor

Here's what I understand:

  1. The issue with openexr for field3d could not be handled for some time.
  2. oiio has removed the necessary dependencies openexr.

So, I think field3d is fine so far as long as we don't update it. For updating openexr, we should do it after the upstream has dealt with the field3d issue.
For oiio, we can move openexr from a required dependency as a feature, or simply remove this dependency in the next update of oiio.

@fabiencastan
Copy link
Contributor

fabiencastan commented Jan 11, 2022

The main problem is that openexr is a highly used packages in the VFX industry (http://vfxplatform.com is documenting the recommended versions for the VFX industry). OpenEXR needs to be updated to 3.x as soon as possible. OpenEXR-2 has conflicts with some boost versions and with cuda (due to "half" type redefinition). These are fixed in OpenEXR-3.

Also, currently vcpkg is broken as there is both an imath package and an old openexr package which includes an old version of imath. So during installation there are 2 versions of imath headers and libraries mixed together.

So, IMO we should drop the "field3d" package and upgrade "openexr".

@JackBoosY
Copy link
Contributor

Also, currently vcpkg is broken as there is both an imath package and an old openexr package which includes an old version of imath. So during installation there are 2 versions of imath headers and libraries mixed together.

After some exploration, I found that ilmbase in vcpkg actually points to ilmbase in openexr, so I think this is not an issue.

The main problem is that openexr is a highly used packages in the VFX industry (http://vfxplatform.com is documenting the recommended versions for the VFX industry). OpenEXR needs to be updated to 3.x as soon as possible. OpenEXR-2 has conflicts with some boost versions and with cuda (due to "half" type redefinition). These are fixed in OpenEXR-3.

Now, openexr doesn't depends on field3d and there is no code related to field3d. Therefore, field3d is no longer in the dependency chain of openexr or oiio.
So I don't think we need to remove field3d.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 12, 2022

Now, openexr doesn't depends on field3d and there is no code related to field3d. Therefore, field3d is no longer in the dependency chain of openexr or oiio.

@JackBoosY It is the other way: field3d depends on openexr. Updating openexr breaks the unmaintained field3d so it can't pass CI.

@JackBoosY
Copy link
Contributor

JackBoosY commented Jan 12, 2022

Now, openexr doesn't depends on field3d and there is no code related to field3d. Therefore, field3d is no longer in the dependency chain of openexr or oiio.

@JackBoosY It is the other way: field3d depends on openexr. Updating openexr breaks the unmaintained field3d so it can't pass CI.

OMG, that's unfortunate. Fine, I personaly agreed that, but please empty this port since we shouldn't "remove" any port, see example: https://github.com/microsoft/vcpkg/tree/030cfaa24de9ea1bbf0a4d9c615ce7312ba77af1/ports/parquet

@strega-nil-ms @BillyONeal what do you think about?

@BillyONeal
Copy link
Member

I think the right thing is to add field3d to ci.baseline.txt. Let me ask around.

@JackBoosY
Copy link
Contributor

I think the right thing is to add field3d to ci.baseline.txt. Let me ask around.

Once openexr update, field3d will no longer be built succesful, does it make sense?

@theblackunknown
Copy link
Contributor

Out of curiosity, can vcpkg versionning feature help to keep a valid field3d port here ?

Either using overrides to pin openexr version to a 2.X

--- C:\devel\vcpkg\ports\field3d\vcpkg.json	Sun Sep 12 15:19:46 2021
+++ C:\devel\vcpkg\ports\field3d\vcpkg.json	Fri Jan 14 09:21:41 2022
@@ -16,6 +16,9 @@
     "boost-timer",
     "hdf5",
     "openexr"
+  ],
+  "overrides": [
+    { "name": "openexr", "version": "2.5.0" }
   ]
 }

Or a builtin-baseline so that the right version is picked

--- C:\devel\vcpkg\ports\field3d\vcpkg.json	Sun Sep 12 15:19:46 2021
+++ C:\devel\vcpkg\ports\field3d\vcpkg.json	Fri Jan 14 09:23:52 2022
@@ -16,6 +16,7 @@
     "boost-timer",
     "hdf5",
     "openexr"
-  ]
+  ],
+  "builtin-baseline":"9f04533f307a8560cc1cc5c1fdee0d88d6d7d818"
 } 

or both ?

If yes that would allow to keep a functional field3d port, possibly useful for backwards compatibility instead of an empty port

@chausner
Copy link
Contributor Author

I think the right thing is to add field3d to ci.baseline.txt. Let me ask around.

Thanks, please let me know once there is a decision how to best handle this case.

@JackBoosY
Copy link
Contributor

@theblackunknown No, vcpkg will no pick the required dependency version, only use the current latest version in vcpkg.

@theblackunknown
Copy link
Contributor

@JackBoosY Understood, so versioning is only useful when describing dependencies for projects correct ?
In any case, thanks for the explanation !

@JackBoosY
Copy link
Contributor

@JackBoosY Understood, so versioning is only useful when describing dependencies for projects correct ? In any case, thanks for the explanation !

  • In traditional mode(command mode), version control only follows the description in vcpkg.json. If it is not explicitly stated in vcpkg.json, the current latest version is used in vcpkg.
  • In manifest mode, the user can enable versioning by declaring this rule in the private vcpkg.json.

@fabiencastan
Copy link
Contributor

Hi all,
What's the current status? Are we close to getting this PR merged?

@JackBoosY
Copy link
Contributor

@BillyONeal Any results?

@fabiencastan
Copy link
Contributor

It is a critical blocking point for us for weeks.
Is there any chance that this package could be removed soon to finally update openexr?
Thanks for your help.

# Conflicts:
#	ports/field3d/portfile.cmake
#	ports/field3d/vcpkg.json
#	ports/openimageio/vcpkg.json
#	versions/baseline.json
#	versions/o-/openimageio.json
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!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 5fe063fda0392f35719dac96a6b236117652f8aa -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index cf61f8f..9b8a93c 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -2168,6 +2168,10 @@
       "baseline": "2019-12-19",
       "port-version": 2
     },
+    "field3d": {
+      "baseline": "1.7.3",
+      "port-version": 3
+    },
     "fixed-string": {
       "baseline": "0.1.0",
       "port-version": 1

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/openimageio/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

@JackBoosY JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Feb 8, 2022
@BillyONeal
Copy link
Member

Turns out there was a bit of a "wires crossing" here; our old rules were "no removals ever"; the new rules are "if a port is completely broken, its contents and baseline can be deleted because people can access old versions using the versions database. The versions DB must be kept though".

I resolved the merge conflicts and now think this is OK but want confirmation from another maintainer before merging because removals are impactful.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Feb 8, 2022
@ras0219-msft ras0219-msft added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Feb 9, 2022
@ras0219-msft ras0219-msft merged commit 5662ef4 into microsoft:master Feb 10, 2022
ekilmer added a commit to ekilmer/vcpkg that referenced this pull request Feb 15, 2022
* master: (54 commits)
  [imgui] Update to 1.87 [implot] Update to 0.13 (microsoft#22988)
  [nu-book-zxing-cpp] New port  (microsoft#22657)
  [librabbitmq] Update to 0.11.0 (microsoft#23037)
  [doc] Add doc for `supports` expression `staticcrt` (microsoft#23079)
  [doctest] Update to 2.4.8 (microsoft#23081)
  [log4cplus] Remove unneeded catch dependency (microsoft#23066)
  [Azure SDK] Update vcpkg ports for Feb Release (microsoft#23080)
  [Freerdp] Update to 2.5.0 (microsoft#23095)
  Update vcpkg-tool to 2022-02-11 (microsoft#23059)
  Minor bugfixes to MacOS deployment readme. (microsoft#23062)
  [ci.baseline.txt] Skip colmap on osx due to metis conflict (microsoft#23047)
  [gtkmm] update to 4.6.0 (microsoft#23024)
  [faiss] Update to 1.7.2 (microsoft#22705)
  [ocilib] Disable warning C4191 (microsoft#23028)
  [polyhook2] Update to latest  (microsoft#23044)
  Add notice about how to export unofficial CMake targets. (microsoft#23041)
  [Spirv reflect] Add new port (microsoft#22295)
  [easyhook] Update target .NET Framework version to 4.7.2. (microsoft#23040)
  [gh suggestions] change license link, make it details (microsoft#22946)
  [field3d] Remove port (microsoft#22463)
  ...
@fabiencastan
Copy link
Contributor

Thank you!!

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 requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants