Skip to content

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

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.

Sign up for GitHub

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

Closed
dg0yt wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
584cfe2
Rewrite to single invovation of 'vcpkg ci'
dg0yt Jul 30, 2021
2c16025
Add AZP errors for skipped ports (CI baseline or cascade)
dg0yt Jul 30, 2021
e71f8df
Catch CASCADED_DUE_TO_MISSING_DEPENDENCIES
dg0yt Aug 14, 2021
0af40b1
Proof-of-concept: HEAD~10 (DO NOT MERGE)
dg0yt Aug 14, 2021
ed98a81
[skip actions] Proof-of-concept: CASCADED_DUE_TO_MISSING_DEPENDENCIES…
dg0yt Aug 15, 2021
df1465b
[skip actions] Revise messages
dg0yt Aug 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ports/freexl/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
message(FATAL_ERROR "Simulate failure")
Copy link
Contributor

@ras0219-msft ras0219-msft Sep 9, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dg0yt dg0yt Sep 10, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ras0219-msft ras0219-msft Sep 12, 2021

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@dg0yt dg0yt Sep 17, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dg0yt dg0yt Oct 7, 2021

Choose a reason for hiding this comment

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

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


set(FREEXL_VERSION_STR "1.0.4")

vcpkg_download_distfile(ARCHIVE
Expand Down
2 changes: 2 additions & 0 deletions ports/spatialite-tools/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
message(FATAL_ERROR "Should not build, CASCADED_DUE_TO_MISSING_DEPENDENCIES (freexl)")

set(SPATIALITE_TOOLS_VERSION_STR "5.0.0")
vcpkg_download_distfile(ARCHIVE
URLS "http://www.gaia-gis.it/gaia-sins/spatialite-tools-sources/spatialite-tools-${SPATIALITE_TOOLS_VERSION_STR}.tar.gz"
Expand Down
54 changes: 49 additions & 5 deletions scripts/azure-pipelines/test-modified-ports.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,16 +142,60 @@ $skipList = . "$PSScriptRoot/generate-skip-list.ps1" `
-BaselineFile "$PSScriptRoot/../ci.baseline.txt" `
-SkipFailures:$skipFailures

$LogSkippedPorts = $true # Maybe parameter
$changedPorts = @()
$skippedPorts = @()
if ($LogSkippedPorts) {
$diffFile = Join-Path $WorkingRoot 'changed-ports.diff'
Start-Process -FilePath 'git' -ArgumentList 'diff --name-only HEAD~10 -- versions ports' `
-NoNewWindow -Wait `
-RedirectStandardOutput $diffFile
$changedPorts = (Get-Content -Path $diffFile) `
-match '^ports/|^versions/.-/' `
-replace '^(ports/|versions/.-/)([^/]*)(/.*|[.]json$)','$2' `
| Sort-Object -Unique
$skippedPorts = $skipList -Split ','
$changedPorts | ForEach-Object {
if ($skippedPorts -contains $_) {
$port = $_
Write-Host "##vso[task.logissue type=error]$port`: build skipped, reason: CI baseline file."
Copy link
Member

@BillyONeal BillyONeal Sep 7, 2021

Choose a reason for hiding this comment

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

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."

Copy link
Member

@BillyONeal BillyONeal Sep 7, 2021

Choose a reason for hiding this comment

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

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

}
}
}

$hostArgs = @()
if ($Triplet -in @('x64-windows', 'x64-osx', 'x64-linux'))
{
# WORKAROUND: These triplets are native-targetting which triggers an issue in how vcpkg handles the skip list.
# The workaround is to pass the skip list as host-excludes as well.
& "./vcpkg$executableExtension" ci $Triplet --x-xunit=$xmlFile --exclude=$skipList --host-exclude=$skipList --failure-logs=$failureLogs @commonArgs
}
else
{
& "./vcpkg$executableExtension" ci $Triplet --x-xunit=$xmlFile --exclude=$skipList --failure-logs=$failureLogs @commonArgs
$hostArgs = @("--host-exclude=$skipList")
}

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

@BillyONeal BillyONeal Sep 7, 2021

Choose a reason for hiding this comment

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

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.

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

@BillyONeal BillyONeal Sep 7, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@BillyONeal BillyONeal Sep 7, 2021

Choose a reason for hiding this comment

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

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]) {

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

@BillyONeal BillyONeal Sep 7, 2021

Choose a reason for hiding this comment

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

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."

}
elseif ($_ -match '^Building package ([^ ]+)[.][.][.]') {
$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 {
Copy link
Contributor

@ras0219-msft ras0219-msft Sep 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ras0219-msft ras0219-msft Sep 9, 2021

Choose a reason for hiding this comment

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

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

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."
Copy link
Member

@BillyONeal BillyONeal Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@BillyONeal BillyONeal Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@BillyONeal BillyONeal Sep 7, 2021

Choose a reason for hiding this comment

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

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."

?

Copy link
Contributor Author

@dg0yt dg0yt Sep 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@BillyONeal BillyONeal Sep 9, 2021

Choose a reason for hiding this comment

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

How about:

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

}
}
}
}
}
}

& "$PSScriptRoot/analyze-test-results.ps1" -logDir $xmlResults `
-triplet $Triplet `
-baselineFile .\scripts\ci.baseline.txt `
Expand Down