Skip to content

[zziplib] Update to v0.13.72#24011

Merged
strega-nil-ms merged 5 commits intomicrosoft:masterfrom
dg0yt:zziplib
Apr 8, 2022
Merged

[zziplib] Update to v0.13.72#24011
strega-nil-ms merged 5 commits intomicrosoft:masterfrom
dg0yt:zziplib

Conversation

@dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Apr 7, 2022

  • What does your PR fix?

    Updates and modernize zziplib.
    Fixes error in the GH Actions versions check.

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

    unchanged, no

  • 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

@JonLiu1993 JonLiu1993 self-assigned this Apr 7, 2022
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Apr 7, 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.

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.

Consider using another version scheme instead of "version-string" in port "zziplib".
Use `--skip-version-format-check` to disable this check.
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 436d98da7f6f06c8dc6bfce2801c543d7cd47ee1 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 3e1afbc..24feb2a 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -7697,8 +7697,8 @@
       "port-version": 3
     },
     "zziplib": {
-      "baseline": "0.13.71",
-      "port-version": 3
+      "baseline": "0.13.72",
+      "port-version": 0
     }
   }
 }
diff --git a/versions/z-/zziplib.json b/versions/z-/zziplib.json
index 708656c..64aa3a4 100644
--- a/versions/z-/zziplib.json
+++ b/versions/z-/zziplib.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "bb3c9f9d50c2a93471726d243892a9a18b9bef96",
+      "version-string": "0.13.72",
+      "port-version": 0
+    },
     {
       "git-tree": "af9957469a45f5b512845c4f180af1a7e4e2e886",
       "version-string": "0.13.71",

You have modified or added at least one vcpkg.json where a "license" field is missing.

Details

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

  • ports/zziplib/vcpkg.json

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

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.

Consider using another version scheme instead of "version-string" in port "zziplib".
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 436d98da7f6f06c8dc6bfce2801c543d7cd47ee1 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 3e1afbc..24feb2a 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -7697,8 +7697,8 @@
       "port-version": 3
     },
     "zziplib": {
-      "baseline": "0.13.71",
-      "port-version": 3
+      "baseline": "0.13.72",
+      "port-version": 0
     }
   }
 }
diff --git a/versions/z-/zziplib.json b/versions/z-/zziplib.json
index 708656c..64aa3a4 100644
--- a/versions/z-/zziplib.json
+++ b/versions/z-/zziplib.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "bb3c9f9d50c2a93471726d243892a9a18b9bef96",
+      "version-string": "0.13.72",
+      "port-version": 0
+    },
     {
       "git-tree": "af9957469a45f5b512845c4f180af1a7e4e2e886",
       "version-string": "0.13.71",

You have modified or added at least one vcpkg.json where a "license" field is missing.

Details

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

  • ports/zziplib/vcpkg.json

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

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 7, 2022

(Edited after #24011 (comment).)

CC @autoantwort @strega-nil-ms @ras0219-msft I picked this port to address issues with the formatting and version check:

  • The action fails with no output when version scheme version-string is used. (Cf. GH actions for first push, with
    "version-string": "0.13.72" (20b5c35)
  • vcpkg tool blindly suggest to use scheme version even if the value doesn't match the requirements for this scheme (e.g. 1de9c66),
    When the version-string matches another scheme, vcpkg tool suggests to switch and returns with error. This triggers the GH actions error. Locally, there is the following output:
    Use the version scheme "version" instead of "version-string" in port "zziplib".
    Use `--skip-version-format-check` to disable this check.
    
  • Reviewers blindly suggested the same to other contributors.

This PR now fixes that situation, by running vcpkg x-add-version once without and once with --skip-version-format-check.
and by modifying the output (cf. #24011 (review)).

This is offered as an immediate mitigation. It might be necessary to make changes to the tool.

@dg0yt dg0yt marked this pull request as ready for review April 7, 2022 06:49
@JackBoosY JackBoosY added the category:infrastructure Pertaining to the CI/Testing infrastrucutre label Apr 7, 2022
@JackBoosY JackBoosY requested a review from BillyONeal April 7, 2022 09:39
Copy link
Contributor

Choose a reason for hiding this comment

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

If upstream rejects this change, I'd like to approve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was previously in VCPKG_C_FLAGS in the portfile. But VCPKG_C_FLAGS depends on the chainloaded toolchain, and the portfile lack the windows condition.
So either accept it here, or I will restore the portfile code.

@autoantwort
Copy link
Contributor

vcpkg tool blindly suggest to use scheme version even if the value doesn't match the requirements for this scheme

That is not true. See
https://github.com/microsoft/vcpkg-tool/blob/0840ffdf10c20c23fc5b9a048e1d7f896e3e76b1/src/vcpkg/commands.add-version.cpp#L70-L93

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 7, 2022

That is not true.

I see, and I apologize. I could verify that now with the current tool and port clockutils: 1.1.1-3651f232c27074c4ceead169e223edf5f00247c5 is accepted as version. I should have verified before posting, but I remembered a different observation.

This means I should restore the original recommendation message. A fix to the action is still needed, in order to show the recommendation instead of an error. And it should go to a different headline than "PRs must add only one version and must not modify any published versions".

Copy link
Contributor

Choose a reason for hiding this comment

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

what's up with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It fixes the broken GH Action when running into version-string.

@strega-nil-ms
Copy link
Contributor

@dg0yt I have no idea what you mean; 0.13.72 is a totally fine "version"

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 7, 2022

@dg0yt I have no idea what you mean; 0.13.72 is a totally fine "version"

@strega-nil-ms I was concerned about "version-string": "1.1.1-3651f232c27074c4ceead169e223edf5f00247c5", as touched in another PR.

With all these literals being acceptable versions, the GH Action should really pass the tool's proposal, not exit with error.
Demonstrating and fixing the GH Action was the main motivation of this PR. I will update it again in a way that verifies the desired change.

@dg0yt dg0yt marked this pull request as draft April 7, 2022 18:33
@dg0yt dg0yt force-pushed the zziplib branch 4 times, most recently from 7e87829 to cac8653 Compare April 8, 2022 05:06
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 d72783cb3aeddfd667861caef1060e54ca6fa7a9 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 4dc8f8d..b92593d 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -7705,8 +7705,8 @@
       "port-version": 3
     },
     "zziplib": {
-      "baseline": "0.13.71",
-      "port-version": 3
+      "baseline": "0.13.72",
+      "port-version": 0
     }
   }
 }
diff --git a/versions/z-/zziplib.json b/versions/z-/zziplib.json
index 708656c..64aa3a4 100644
--- a/versions/z-/zziplib.json
+++ b/versions/z-/zziplib.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "bb3c9f9d50c2a93471726d243892a9a18b9bef96",
+      "version-string": "0.13.72",
+      "port-version": 0
+    },
     {
       "git-tree": "af9957469a45f5b512845c4f180af1a7e4e2e886",
       "version-string": "0.13.71",

You have modified or added at least one vcpkg.json where a "license" field is missing.

Details

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

  • ports/zziplib/vcpkg.json

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

Separately collect and display result from version scheme check.
Ignore error from version scheme check when checking version values.
Pass output also to console.
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!

Use the version scheme "version" instead of "version-string" in port "zziplib".

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout d72783cb3aeddfd667861caef1060e54ca6fa7a9 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 4dc8f8d..b92593d 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -7705,8 +7705,8 @@
       "port-version": 3
     },
     "zziplib": {
-      "baseline": "0.13.71",
-      "port-version": 3
+      "baseline": "0.13.72",
+      "port-version": 0
     }
   }
 }
diff --git a/versions/z-/zziplib.json b/versions/z-/zziplib.json
index 708656c..64aa3a4 100644
--- a/versions/z-/zziplib.json
+++ b/versions/z-/zziplib.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "bb3c9f9d50c2a93471726d243892a9a18b9bef96",
+      "version-string": "0.13.72",
+      "port-version": 0
+    },
     {
       "git-tree": "af9957469a45f5b512845c4f180af1a7e4e2e886",
       "version-string": "0.13.71",

You have modified or added at least one vcpkg.json where a "license" field is missing.

Details

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

  • ports/zziplib/vcpkg.json

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

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 8, 2022

Use the version scheme "version" instead of "version-string" in port "zziplib".

🎉

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 d72783cb3aeddfd667861caef1060e54ca6fa7a9 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 4dc8f8d..b92593d 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -7705,8 +7705,8 @@
       "port-version": 3
     },
     "zziplib": {
-      "baseline": "0.13.71",
-      "port-version": 3
+      "baseline": "0.13.72",
+      "port-version": 0
     }
   }
 }
diff --git a/versions/z-/zziplib.json b/versions/z-/zziplib.json
index 708656c..3aec32b 100644
--- a/versions/z-/zziplib.json
+++ b/versions/z-/zziplib.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "e09e8bf85eff5c115f9dcf5372f8bdbab590ab6b",
+      "version": "0.13.72",
+      "port-version": 0
+    },
     {
       "git-tree": "af9957469a45f5b512845c4f180af1a7e4e2e886",
       "version-string": "0.13.71",

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 8, 2022

Now the GH Action no longer fails, and it captures the messages as needed.

  • I decided to not block "approval" in presence of a version-string - I'm not sure if there might be valid cases for staying with that scheme even if the relaxed version scheme "matches".
  • It would be possible to generate a diff for the proposed version scheme change. However, it would need to handle two scheme already now (version, version-date), so it is more than one extra line.

@dg0yt dg0yt marked this pull request as ready for review April 8, 2022 05:31
@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 8, 2022

🎉 llvm:x64-windows restored from cache, finally. (#23896)

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 8, 2022

I remembered a different observation.

FTR it was here #23532 (comment). But this was also just based on assumptions, and in practice, 164-5db6aa6cab1b146 is accepted as version.

@JackBoosY JackBoosY requested a review from strega-nil-ms April 8, 2022 07:59
@JackBoosY JackBoosY added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed requires:author-response labels Apr 8, 2022
@strega-nil
Copy link
Contributor

I disagree strongly with the "version-string" change (and at least it should be its own PR) - "version-string" is a back-compat hack that should almost never be used.

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 8, 2022

I disagree strongly with the "version-string" change

Which change?

(and at least it should be its own PR)

I can split the PR for final history, but I had to verify that it works. Unfortunately, I still didn't get the message through, despite providing the demonstration in the history of this PR.

"version-string" is a back-compat hack that should almost never be used.

So where is the problem?

  • Current CI behaviour: the PR suggestion action fails if there is version-string. User confused.
  • New CI behaviour: the PR suggestion passes the tool's suggestion to replace version-string. User can act.

@strega-nil
Copy link
Contributor

strega-nil commented Apr 8, 2022

Oh shit! I totally missed what it was actually doing, sorry. Thanks!

@strega-nil-ms strega-nil-ms merged commit 621e156 into microsoft:master Apr 8, 2022
@dg0yt dg0yt deleted the zziplib branch April 8, 2022 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:infrastructure Pertaining to the CI/Testing infrastrucutre category:port-update The issue is with a library, which is requesting update new revision 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.

6 participants