[vcpkg-ci] Add AZP logging markup for skipped ports #19259

dg0yt · 2021-07-30T21:13:56Z

What does your PR fix?

Occassionally, skipped builds (in particular "cascade") for changed ports went unnoticed.
Based on changed ports and versions files in git, this change adds AZP error messages (but not job failures) for skipped ports, making them more visible in Github.
Which triplets are supported/not supported? Have you updated the CI baseline?

all, not needed
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?

not needed

dg0yt · 2021-08-01T14:03:22Z

Works now as intented, but includes a POC commit which must be removed before merge.

github-actions

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 6bc4362fb49e53f1fff7f51e4e27e1946755ecc6 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/baseline.json b/versions/baseline.json
index 94f34ac..02a5321 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -5530,7 +5530,7 @@
     },
     "raylib": {
       "baseline": "3.7.0",
-      "port-version": 1
+      "port-version": 2
     },
     "rbdl": {
       "baseline": "2.6.0",
diff --git a/versions/r-/raylib.json b/versions/r-/raylib.json
index 493c251..70bc6a8 100644
--- a/versions/r-/raylib.json
+++ b/versions/r-/raylib.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "a6e75fbb346a36aecf774e698ac01e7d9122632b",
+      "version-semver": "3.7.0",
+      "port-version": 2
+    },
     {
       "git-tree": "e27352fbab2a4e815a478265032a4faa60d34060",
       "version-semver": "3.7.0",

github-actions

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 6bc4362fb49e53f1fff7f51e4e27e1946755ecc6 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/baseline.json b/versions/baseline.json
index 94f34ac..02a5321 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -5530,7 +5530,7 @@
     },
     "raylib": {
       "baseline": "3.7.0",
-      "port-version": 1
+      "port-version": 2
     },
     "rbdl": {
       "baseline": "2.6.0",
diff --git a/versions/r-/raylib.json b/versions/r-/raylib.json
index 493c251..7f97573 100644
--- a/versions/r-/raylib.json
+++ b/versions/r-/raylib.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "8ea2085bf2813963d5e4a881b09dab3017bf92b3",
+      "version-semver": "3.7.0",
+      "port-version": 2
+    },
     {
       "git-tree": "e27352fbab2a4e815a478265032a4faa60d34060",
       "version-semver": "3.7.0",

… (DO NOT MERGE)

dg0yt · 2021-08-15T08:07:17Z

This now also covers skipped ports which depend on changed ports.

It should be reviewed and (after removal of the POC commits) merged for the benefit of all contributors.

PhoebeHui · 2021-08-23T08:49:06Z

@dg0yt, I will ask other members to review this PR! Sorry for the delay!

dg0yt · 2021-09-05T15:23:55Z

Ping for hints how to move this forward.

PhoebeHui · 2021-09-06T09:00:34Z

@ras0219 @ras0219-msft @BillyONeal, could you please take a look again?

BillyONeal · 2021-09-07T22:28:22Z

scripts/azure-pipelines/test-modified-ports.ps1

+    $changedPorts | ForEach-Object {
+        if ($skippedPorts -contains $_) {
+            $port = $_
+            Write-Host "##vso[task.logissue type=error]$port`: build skipped, reason: CI baseline file."


Suggested change

Write-Host "##vso[task.logissue type=error]$port`: build skipped, reason: CI baseline file."

Write-Host "##vso[task.logissue type=error]$port`: skipped by ci.baseline.txt."

I think it would be good to use the file name here so that contributors can find it more easily.

BillyONeal · 2021-09-07T22:29:16Z

scripts/azure-pipelines/test-modified-ports.ps1

+
+$current_port_and_features = ':'
+& "./vcpkg$executableExtension" ci $Triplet --x-xunit=$xmlFile --exclude=$skipList --failure-logs=$failureLogs @hostArgs @commonArgs `
+    | ForEach-Object {


For other reviewers: I expected this to hang the run until all the output was collected but it didn't seem to do that in testing.

BillyONeal · 2021-09-07T22:33:42Z

scripts/azure-pipelines/test-modified-ports.ps1

+    | ForEach-Object {
+        $_
+        if ($LogSkippedPorts) {
+            if ($_ -match '^ *([^ :]+):[^:]*: *cascade:' -and $changedPorts -contains $Matches[1]) {


I'm a little concerned about making this "output intended for human consumption" load bearing in this way. Not an over my dead body issue but a better solution would be to have the tool do this processing and report it in the output so that changes in the tool don't cause silent failure of the infrastructure.

That said, while that is a solution I prefer, this solution is better than the status quo and such failure would only degrade things to the status quo, so I would not block merging over that.

Suggested change

if ($_ -match '^ *([^ :]+):[^:]*: *cascade:' -and $changedPorts -contains $Matches[1]) {

# Parse the ports ABI hash list at the start of output looking for ports vcpkg ci wants to build

# (denoted by *) which are marked "cascade" because one of their dependencies were in

# ci.baseline.txt.

if ($_ -match '^ *([^ :]+):[^:]*: *cascade:' -and $changedPorts -contains $Matches[1]) {

BillyONeal · 2021-09-07T22:35:22Z

scripts/azure-pipelines/test-modified-ports.ps1

+        if ($LogSkippedPorts) {
+            if ($_ -match '^ *([^ :]+):[^:]*: *cascade:' -and $changedPorts -contains $Matches[1]) {
+                $port = $Matches[1]
+                Write-Host "##vso[task.logissue type=error]$port`: build skipped, reason: cascade from CI baseline file."


Suggested change

Write-Host "##vso[task.logissue type=error]$port`: build skipped, reason: cascade from CI baseline file."

Write-Host "##vso[task.logissue type=error]$port`: build skipped because a dependency is skipped in ci.baseline.txt."

BillyONeal · 2021-09-07T22:38:33Z

scripts/azure-pipelines/test-modified-ports.ps1

+                    if ($_ -match '^([^:[]+)[:[]' -and $changedPorts -contains $Matches[1]) {
+                        $port = $Matches[1]
+                        if ($current_port_and_features -notmatch "^$port[:[]") {
+                            Write-Host "##vso[task.logissue type=error]$port`: build of depending port '$current_port_and_features' skipped due to missing dependencies."


~~One reason it might be a good idea to move this into the tool is that it would be easier to describe which dependency failed.~~

~~I don't duplicate the port name in my suggested edit; I'm on the fence about whether we should avoid the duplication or not.~~

Oh, I see, I had this backwards. It isn't a repeated port name, the actual output is something like:

freexl: build of depending port 'libspatialite[core]:x86-windows' skipped due to missing dependencies.

as in, "freexl failed, and that made libspatialite cascade"

How about:

Write-Host "##vso[task.logissue type=error]$port`: build failure caused '$current_port_and_features' skip due to missing dependencies."

?

Note that we cannot be sure that a freexl build failure causes libspatialite[core]:x86-windows to be skipped. All we know is that:

freexl is direct or transitive dependency of libspatialite[core]:x86-windows.

one of the dependencies of libspatialite[core]:x86-windows failed to build.

How about:

Write-Host "##vso[task.logissue type=error]$current_port_and_features`: not attempted because '$port' is unavailable."

BillyONeal · 2021-09-07T23:09:21Z

I poked other maintainers to point out that despite this P/R being "failing" that is intentional right now to demonstrate output.

ras0219-msft · 2021-09-09T18:46:16Z

scripts/azure-pipelines/test-modified-ports.ps1

+                $current_port_and_features = $Matches[1]
+            }
+            elseif ($_ -match 'failed with: CASCADED_DUE_TO_MISSING_DEPENDENCIES') {
+                & "./vcpkg$executableExtension" depend-info $current_port_and_features | ForEach-Object {


We should save these and process them at the end of the build -- otherwise this is performing concurrent vcpkg calls which is a big no-no.

Alternatively: this might be appropriate to add to the original ci command

ras0219-msft · 2021-09-09T19:01:16Z

ports/freexl/portfile.cmake

@@ -1,3 +1,5 @@
+message(FATAL_ERROR "Simulate failure")


I may misunderstand the purpose of this machinery, but I think the failures this is intended to capture are when something is moving to skip -- i.e. when the supports field or the ci.baseline.txt is modified. If a port fails to build, we'll already see that error as part of the main CI output. It's the reaction to that (add to ci.baseline.txt or change supports) that causes these "invisible" failures.

Therefore, I think it would be a better test of this machinery would be to change freexl's supports clause.

The case which I wanted to cover here is not obious in the proof-of-concept change:

Independent port A is modified by a PR.

Independent port B fails for some other reason.

Port C which depends on A and B is skipped because of B failing.
@BillyONeal That's why the output pattern is:
A: build of depending port 'C' skipped due to missing dependencies."
-- I'm looking at C, I know that it depends on A, and I know that A was modified.

(In the proof of concept, A = B.)
I admit that this might generate a lot of output if A and B are frequently used ports, in particular if A = B.

Now I could also collect failed ports, and thus identify that B caused C to be skipped.

In addition, it might be possible to directly mark B as "unrelated" error if not modified and not depending on a modified port. However, this doesn't work when modifying maintainer functions in scripts.

Our policy is that we do not generally merge PRs with B failing -- we will wait for another PR to fix B or fix B in this PR. Given that policy, what does this additional check catch?

(In exceptional circumstances we do merge PRs with a port failing, but in those cases we perform additional manual review to ensure that we aren't in the A+B=C case you've outlined)

Our policy is that we do not generally merge PRs with B failing -- we will wait for another PR to fix B or fix B in this PR. Given that policy, what does this additional check catch?

Okay.

With regard to independent ports, the information which would be immediately helpful instead is whether a failing port is unrelated to modified ports. This would reduce workload for contributors and reviewers.
Examples: #20136 (comment)
However, this information would be needed next to the existing message:

##[error]REGRESSION: python3:x64-osx. If expected, add python3:x64-osx=fail to .\scripts\ci.baseline.txt.

The "unrelated ports" are now better resolved by microsoft/vcpkg-tool#210.

dg0yt · 2022-03-25T07:47:39Z

I won't continue this PR. The changes for AZP test result upload (via vcpkg ci xUnit output) should hopefully help to improve transparency about tests which are "not executed".

dg0yt force-pushed the ci-annotations branch from c92ec16 to b8bcecc Compare July 30, 2021 21:17

github-actions bot approved these changes Jul 30, 2021

View reviewed changes

dg0yt force-pushed the ci-annotations branch from 4507052 to d18cb76 Compare July 30, 2021 21:34

github-actions bot approved these changes Jul 30, 2021

View reviewed changes

dg0yt force-pushed the ci-annotations branch from d18cb76 to f9feeae Compare July 30, 2021 21:41

github-actions bot approved these changes Jul 30, 2021

View reviewed changes

dg0yt force-pushed the ci-annotations branch from f9feeae to e80f341 Compare July 31, 2021 04:43

github-actions bot approved these changes Jul 31, 2021

View reviewed changes

dg0yt force-pushed the ci-annotations branch from e80f341 to b2d5047 Compare July 31, 2021 05:02

github-actions bot approved these changes Jul 31, 2021

View reviewed changes

dg0yt force-pushed the ci-annotations branch from b2d5047 to 34839bf Compare July 31, 2021 05:36

github-actions bot approved these changes Jul 31, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

dg0yt force-pushed the ci-annotations branch from 34839bf to 9822508 Compare August 1, 2021 12:48

github-actions bot approved these changes Aug 1, 2021

View reviewed changes

dg0yt force-pushed the ci-annotations branch from 9822508 to 116ff05 Compare August 1, 2021 13:22

github-actions bot approved these changes Aug 1, 2021

View reviewed changes

dg0yt force-pushed the ci-annotations branch from 4da69ec to 74f3644 Compare August 1, 2021 13:56

github-actions bot approved these changes Aug 1, 2021

View reviewed changes

dg0yt marked this pull request as ready for review August 1, 2021 14:03

dg0yt force-pushed the ci-annotations branch from 74f3644 to e8dd2d6 Compare August 1, 2021 14:33

github-actions bot approved these changes Aug 1, 2021

View reviewed changes

JonLiu1993 assigned PhoebeHui Aug 2, 2021

PhoebeHui added category:infrastructure Pertaining to the CI/Testing infrastrucutre requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Aug 2, 2021

dg0yt force-pushed the ci-annotations branch from 80abb3a to f2e8e84 Compare August 14, 2021 19:39

github-actions bot requested changes Aug 14, 2021

View reviewed changes

dg0yt force-pushed the ci-annotations branch from f2e8e84 to 0916229 Compare August 14, 2021 19:54

github-actions bot requested changes Aug 14, 2021

View reviewed changes

dg0yt force-pushed the ci-annotations branch from 0916229 to 5fbde7f Compare August 14, 2021 20:18

dg0yt added 4 commits August 14, 2021 22:36

Rewrite to single invovation of 'vcpkg ci'

584cfe2

Add AZP errors for skipped ports (CI baseline or cascade)

2c16025

Catch CASCADED_DUE_TO_MISSING_DEPENDENCIES

e71f8df

Proof-of-concept: HEAD~10 (DO NOT MERGE)

0af40b1

dg0yt force-pushed the ci-annotations branch from 5fbde7f to 3c247c2 Compare August 14, 2021 20:36

github-actions bot approved these changes Aug 14, 2021

View reviewed changes

dg0yt force-pushed the ci-annotations branch from 3c247c2 to 0af40b1 Compare August 14, 2021 20:41

github-actions bot approved these changes Aug 14, 2021

View reviewed changes

dg0yt added 2 commits August 15, 2021 08:08

[skip actions] Proof-of-concept: CASCADED_DUE_TO_MISSING_DEPENDENCIES…

ed98a81

… (DO NOT MERGE)

[skip actions] Revise messages

df1465b

dg0yt force-pushed the ci-annotations branch from 80f9aba to df1465b Compare August 15, 2021 07:55

BillyONeal reviewed Sep 7, 2021

View reviewed changes

dg0yt mentioned this pull request Sep 9, 2021

[VTK] fix cmake patches not recycling targets already defined #20074

Merged

ras0219-msft reviewed Sep 9, 2021

View reviewed changes

Cheney-W added the requires:author-response label Sep 27, 2021

JackBoosY removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 30, 2021

dg0yt marked this pull request as draft October 25, 2021 06:28

dg0yt closed this Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vcpkg-ci] Add AZP logging markup for skipped ports #19259

[vcpkg-ci] Add AZP logging markup for skipped ports #19259

dg0yt commented Jul 30, 2021 •

edited

Loading

This comment has been minimized.

dg0yt commented Aug 1, 2021

github-actions bot left a comment

github-actions bot left a comment

dg0yt commented Aug 15, 2021

PhoebeHui commented Aug 23, 2021

dg0yt commented Sep 5, 2021

PhoebeHui commented Sep 6, 2021

BillyONeal Sep 7, 2021

BillyONeal Sep 7, 2021

BillyONeal Sep 7, 2021

BillyONeal Sep 7, 2021

BillyONeal Sep 7, 2021

BillyONeal Sep 7, 2021

BillyONeal Sep 7, 2021 •

edited

Loading

BillyONeal Sep 7, 2021 •

edited

Loading

BillyONeal Sep 7, 2021

dg0yt Sep 8, 2021

BillyONeal Sep 9, 2021

BillyONeal commented Sep 7, 2021

ras0219-msft Sep 9, 2021 •

edited

Loading

ras0219-msft Sep 9, 2021

ras0219-msft Sep 9, 2021

dg0yt Sep 10, 2021

ras0219-msft Sep 12, 2021

dg0yt Sep 17, 2021

dg0yt Oct 7, 2021

dg0yt commented Mar 25, 2022

	Write-Host "##vso[task.logissue type=error]$port`: build skipped, reason: CI baseline file."
	Write-Host "##vso[task.logissue type=error]$port`: skipped by ci.baseline.txt."

-            if ($_ -match '^ *([^ :]+):[^:]*: *cascade:' -and $changedPorts -contains $Matches[1]) {
+            # Parse the ports ABI hash list at the start of output looking for ports vcpkg ci wants to build
+            # (denoted by *) which are marked "cascade" because one of their dependencies were in
+            # ci.baseline.txt.
+            if ($_ -match '^ *([^ :]+):[^:]*: *cascade:' -and $changedPorts -contains $Matches[1]) {

[vcpkg-ci] Add AZP logging markup for skipped ports #19259

[vcpkg-ci] Add AZP logging markup for skipped ports #19259

Conversation

dg0yt commented Jul 30, 2021 • edited Loading

What does your PR fix?

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

Does your PR follow the maintainer guide?

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

This comment has been minimized.

dg0yt commented Aug 1, 2021

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

dg0yt commented Aug 15, 2021

PhoebeHui commented Aug 23, 2021

dg0yt commented Sep 5, 2021

PhoebeHui commented Sep 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

BillyONeal Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented Sep 7, 2021

ras0219-msft Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dg0yt commented Mar 25, 2022

dg0yt commented Jul 30, 2021 •

edited

Loading

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

BillyONeal Sep 7, 2021 •

edited

Loading

BillyONeal Sep 7, 2021 •

edited

Loading

ras0219-msft Sep 9, 2021 •

edited

Loading