[vcpkg] Use a tag file rather than conditional compilation to permanently disable metrics. #15470
Conversation
…ntly disable metrics.
BillyONeal
added
category:vcpkg-feature
| Write-Host $Script:CurrentTest | ||
| ./vcpkg @testArgs | ||
| & "./$vcpkgName" @testArgs | Tee-Object -Variable 'ConsoleOutput' |
There was a problem hiding this comment.
Since this redirects the output, it would be nice if this was combined with Throw-IfFailed/Throw-IfNotFailed and would print the output on unexpected return code.
Perhaps Run-Vcpkg and Run-Vcpkg-Fail? (I'd keep the name Run-Vcpkg for passing to minimize the impact on existing tests)
function Run-Vcpkg-Fail {
# ...
$out = Run-Vcpkg-Base $TestArgs
if ($LASTEXITCODE -eq 0) {
$out
throw "..."
}
return $out
}There was a problem hiding this comment.
Since this redirects the output
It redirects the output into Tee-Object so the output still goes to the screen too.
| System::print2(System::Color::warning, | ||
| "Warning: passed either --printmetrics or --no-printmetrics, but metrics are disabled.\n"); | ||
| } | ||
| auto metrics = Metrics::g_metrics.lock(); |
There was a problem hiding this comment.
Taking this lock over a larger scope can be problematic because if any failures occur, we will attempt to recursively lock. This is because the exit-on-failure code will try to flush the metrics.
There was a problem hiding this comment.
That is just as much a problem in the "before" code as the "after" code as in both cases the lock is held for the entirety of the contained calls.
…ics_define # Conflicts: # scripts/bootstrap.ps1
toolsrc/src/vcpkg.cpp
Outdated
| { | ||
| System::print2(System::Color::warning, | ||
| "Warning: passed either --sendmetrics or --no-sendmetrics, but metrics are disabled.\n"); | ||
| } | ||
|
|
||
| if (args.print_metrics.has_value() && !metrics->metrics_enabled()) | ||
| { | ||
| System::print2(System::Color::warning, | ||
| "Warning: passed either --printmetrics or --no-printmetrics, but metrics are disabled.\n"); |
There was a problem hiding this comment.
It doesn't make sense to me to warn for --no-printmetrics; I feel like these checks should be: !args.print_metrics.value_or(false).
There was a problem hiding this comment.
tbh I don't think --no-printmetrics should exist since there's no world where we would want to print them by default.
There was a problem hiding this comment.
this is fair, but it still should be true for --no-sendmetrics.
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
…PKG_DISABLE_METRICS in microsoft#15470
…PKG_DISABLE_METRICS (#15810) * Restore x-upload-metrics command accidentally disabled by removing VCPKG_DISABLE_METRICS in microsoft/vcpkg#15470 * Didn't save a file.
…ntly disable metrics. (microsoft#15470)
…PKG_DISABLE_METRICS (#15810) * Restore x-upload-metrics command accidentally disabled by removing VCPKG_DISABLE_METRICS in microsoft/vcpkg#15470 * Didn't save a file.
We want to ship a signed binary rather than having the user build vcpkg's binary, and it is better if we can use the same binary for all customers.
We can't use the existing environment variable solution because
./bootstrap -disableMetricsneeds to make that bootstrapped vcpkg act as if metrics were never compiled in in the first place, but we can get the same effect by writing a tag file next to the binary which tells the binary to have that behavior.Also adds e2e tests of the metrics setting.