-
Notifications
You must be signed in to change notification settings - Fork 296
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
Implement published test results built in and reduce console chattiness. #437
Conversation
|
This change deletes the word "package" from several places where it is clear, and deduplicates "starting install" and "installing" messages. Before: ``` PS C:\Dev\vcpkg> .\vcpkg.exe install zlib Computing installation plan... The following packages will be built and installed: zlib[core]:x86-windows -> 1.2.11#13 Detecting compiler hash for triplet x86-windows... Restored 1 packages from C:\Users\bion\AppData\Local\vcpkg\archives in 43.57 ms. Use --debug to see more details. Starting package 1/1: zlib:x86-windows Installing package zlib[core]:x86-windows... Elapsed time for package zlib:x86-windows: 18.41 ms Total elapsed time: 2.266 s The package zlib is compatible with built-in CMake targets: find_package(ZLIB REQUIRED) target_link_libraries(main PRIVATE ZLIB::ZLIB) PS C:\Dev\vcpkg> .\vcpkg.exe remove zlib The following packages will be removed: zlib:x86-windows Removing package zlib:x86-windows... PS C:\Dev\vcpkg> .\vcpkg.exe install zlib --no-binarycaching Computing installation plan... The following packages will be built and installed: zlib[core]:x86-windows -> 1.2.11#13 Detecting compiler hash for triplet x86-windows... Starting package 1/1: zlib:x86-windows Building package zlib[core]:x86-windows... -- Using cached zlib1211.tar.gz. -- Extracting source C:/Dev/vcpkg-downloads/zlib1211.tar.gz -- Applying patch cmake_dont_build_more_than_needed.patch -- Applying patch 0001-Prevent-invalid-inclusions-when-HAVE_-is-set-to-0.patch -- Applying patch add_debug_postfix_on_mingw.patch -- Applying patch 0002-android-build-mingw.patch -- Using source at C:/Dev/vcpkg/buildtrees/zlib/src/1.2.11-e9a093319e.clean -- Configuring x86-windows -- Building x86-windows-dbg -- Building x86-windows-rel -- Installing: C:/Dev/vcpkg/packages/zlib_x86-windows/share/zlib/vcpkg-cmake-wrapper.cmake -- Fixing pkgconfig file: C:/Dev/vcpkg/packages/zlib_x86-windows/lib/pkgconfig/zlib.pc -- Using cached msys-mingw-w64-i686-pkg-config-0.29.2-3-any.pkg.tar.zst. -- Using cached msys-mingw-w64-i686-libwinpthread-git-9.0.0.6373.5be8fcd83-1-any.pkg.tar.zst. -- Using msys root at C:/Dev/vcpkg-downloads/tools/msys2/9a1ec3f33446b195 -- Fixing pkgconfig file: C:/Dev/vcpkg/packages/zlib_x86-windows/debug/lib/pkgconfig/zlib.pc -- Installing: C:/Dev/vcpkg/packages/zlib_x86-windows/share/zlib/copyright -- Performing post-build validation -- Performing post-build validation done Installing package zlib[core]:x86-windows... Elapsed time for package zlib:x86-windows: 4.83 s Total elapsed time: 7.009 s The package zlib is compatible with built-in CMake targets: find_package(ZLIB REQUIRED) target_link_libraries(main PRIVATE ZLIB::ZLIB) PS C:\Dev\vcpkg> .\vcpkg.exe x-set-installed The following packages will be removed: zlib:x86-windows Starting package 1/1: zlib:x86-windows Removing package zlib:x86-windows... Elapsed time for package zlib:x86-windows: 9.93 ms Restored 0 packages from C:\Users\bion\AppData\Local\vcpkg\archives in 92.5 us. Use --debug to see more details. Total elapsed time: 14.01 ms PS C:\Dev\vcpkg> .\vcpkg.exe remove zlib The following packages are not installed, so not removed: zlib:x86-windows Package zlib:x86-windows is not installed PS C:\Dev\vcpkg> ``` After: ``` PS C:\Dev\vcpkg> ..\vcpkg-tool\out\build\x64-Debug\vcpkg.exe install zlib Computing installation plan... The following packages will be built and installed: zlib[core]:x86-windows -> 1.2.11#13 Detecting compiler hash for triplet x86-windows... Restored 1 packages from C:\Users\bion\AppData\Local\vcpkg\archives in 60.79 ms. Use --debug to see more details. Installing zlib:x86-windows (1/1)... Elapsed time for zlib:x86-windows: 20.03 ms Total elapsed time: 2.662 s The package zlib is compatible with built-in CMake targets: find_package(ZLIB REQUIRED) target_link_libraries(main PRIVATE ZLIB::ZLIB) PS C:\Dev\vcpkg> ..\vcpkg-tool\out\build\x64-Debug\vcpkg.exe remove zlib The following packages will be removed: zlib:x86-windows Removing zlib:x86-windows (1/1)... PS C:\Dev\vcpkg> ..\vcpkg-tool\out\build\x64-Debug\vcpkg.exe install zlib --no-binarycaching Computing installation plan... The following packages will be built and installed: zlib[core]:x86-windows -> 1.2.11#13 Detecting compiler hash for triplet x86-windows... Installing zlib:x86-windows (1/1)... Building zlib[core]:x86-windows... -- Using cached zlib1211.tar.gz. -- Cleaning sources at C:/Dev/vcpkg/buildtrees/zlib/src/1.2.11-e9a093319e.clean. Use --editable to skip cleaning for the packages you specify. -- Extracting source C:/Dev/vcpkg-downloads/zlib1211.tar.gz -- Applying patch cmake_dont_build_more_than_needed.patch -- Applying patch 0001-Prevent-invalid-inclusions-when-HAVE_-is-set-to-0.patch -- Applying patch add_debug_postfix_on_mingw.patch -- Applying patch 0002-android-build-mingw.patch -- Using source at C:/Dev/vcpkg/buildtrees/zlib/src/1.2.11-e9a093319e.clean -- Configuring x86-windows -- Building x86-windows-dbg -- Building x86-windows-rel -- Installing: C:/Dev/vcpkg/packages/zlib_x86-windows/share/zlib/vcpkg-cmake-wrapper.cmake -- Fixing pkgconfig file: C:/Dev/vcpkg/packages/zlib_x86-windows/lib/pkgconfig/zlib.pc -- Using cached msys-mingw-w64-i686-pkg-config-0.29.2-3-any.pkg.tar.zst. -- Using cached msys-mingw-w64-i686-libwinpthread-git-9.0.0.6373.5be8fcd83-1-any.pkg.tar.zst. -- Using msys root at C:/Dev/vcpkg-downloads/tools/msys2/9a1ec3f33446b195 -- Fixing pkgconfig file: C:/Dev/vcpkg/packages/zlib_x86-windows/debug/lib/pkgconfig/zlib.pc -- Installing: C:/Dev/vcpkg/packages/zlib_x86-windows/share/zlib/copyright -- Performing post-build validation -- Performing post-build validation done Elapsed time for zlib:x86-windows: 5.137 s Total elapsed time: 7.36 s The package zlib is compatible with built-in CMake targets: find_package(ZLIB REQUIRED) target_link_libraries(main PRIVATE ZLIB::ZLIB) PS C:\Dev\vcpkg> ..\vcpkg-tool\out\build\x64-Debug\vcpkg.exe remove zlib The following packages will be removed: zlib:x86-windows Removing zlib:x86-windows (1/1)... PS C:\Dev\vcpkg> ..\vcpkg-tool\out\build\x64-Debug\vcpkg.exe remove zlib The following packages are not installed, so not removed: zlib:x86-windows Removing zlib:x86-windows (1/1)... PS C:\Dev\vcpkg> ```
Needs tests.
# Conflicts: # src/vcpkg/build.cpp # src/vcpkg/install.cpp
@@ -25,15 +27,19 @@ namespace vcpkg::Install | |||
|
|||
struct SpecSummary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long term, I think SpecSummary should be "freestanding" and not require the action plan that produced it to be held "alive" in order to be meaningful but that turned out to be more effort than I was willing to spend here. This at least goes somewhat in that direction.
Message diff, since there's much more unchanged text than changed text. diff --git a/t1.txt b/t2.txt
index cb2874848..91f7d2012 100644
--- a/t1.txt
+++ b/t2.txt
@@ -4,9 +4,8 @@ The following packages will be built and installed:
zlib[core]:x86-windows -> 1.2.11#13
Detecting compiler hash for triplet x86-windows...
Restored 1 packages from C:\Users\bion\AppData\Local\vcpkg\archives in x ms. Use --debug to see more details.
-Starting package 1/1: zlib:x86-windows
-Installing package zlib[core]:x86-windows...
-Elapsed time for package zlib:x86-windows: x ms
+Installing zlib:x86-windows (1/1)...
+Elapsed time for zlib:x86-windows: x ms
Total elapsed time: x s
@@ -18,14 +17,14 @@ The package zlib is compatible with built-in CMake targets:
PS C:\Dev\vcpkg> .\vcpkg.exe remove zlib
The following packages will be removed:
zlib:x86-windows
-Removing package zlib:x86-windows...
+Removing zlib:x86-windows (1/1)...
PS C:\Dev\vcpkg> .\vcpkg.exe install zlib --no-binarycaching
Computing installation plan...
The following packages will be built and installed:
zlib[core]:x86-windows -> 1.2.11#13
Detecting compiler hash for triplet x86-windows...
-Starting package 1/1: zlib:x86-windows
-Building package zlib[core]:x86-windows...
+Installing zlib:x86-windows (1/1)...
+Building zlib[core]:x86-windows...
-- Using cached zlib1211.tar.gz.
-- Extracting source C:/Dev/vcpkg-downloads/zlib1211.tar.gz
-- Applying patch cmake_dont_build_more_than_needed.patch
@@ -45,8 +44,7 @@ Building package zlib[core]:x86-windows...
-- Installing: C:/Dev/vcpkg/packages/zlib_x86-windows/share/zlib/copyright
-- Performing post-build validation
-- Performing post-build validation done
-Installing package zlib[core]:x86-windows...
-Elapsed time for package zlib:x86-windows: x s
+Elapsed time for zlib:x86-windows: x s
Total elapsed time: x s
@@ -55,18 +53,12 @@ The package zlib is compatible with built-in CMake targets:
find_package(ZLIB REQUIRED)
target_link_libraries(main PRIVATE ZLIB::ZLIB)
-PS C:\Dev\vcpkg> .\vcpkg.exe x-set-installed
+PS C:\Dev\vcpkg> .\vcpkg.exe remove zlib
The following packages will be removed:
zlib:x86-windows
-Starting package 1/1: zlib:x86-windows
-Removing package zlib:x86-windows...
-Elapsed time for package zlib:x86-windows: x ms
-Restored 0 packages from C:\Users\bion\AppData\Local\vcpkg\archives in x us. Use --debug to see more details.
-
-Total elapsed time: x ms
-
+Removing zlib:x86-windows (1/1)...
PS C:\Dev\vcpkg> .\vcpkg.exe remove zlib
The following packages are not installed, so not removed:
zlib:x86-windows
-Package zlib:x86-windows is not installed
+Removing zlib:x86-windows (1/1)...
PS C:\Dev\vcpkg>
\ No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please localize while you're touching these files; approved post that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I converted the xUnit generator to use our XmlSerializer machinery.
I also pushed localization for all the touched messages.
@@ -90,6 +90,16 @@ namespace vcpkg | |||
} | |||
return *this; | |||
} | |||
XmlSerializer& XmlSerializer::cdata(StringView sv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ras0219-msft I think this CDATA thing was a workaround for not having real XmlSerializer available at the time; should this go through data() instead now that you have proper escapes implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec actually says "as a CDATA block": https://xunit.net/docs/format-xml-v2
(for the <message> child of a <failure> node)
The failure message as a CDATA block.
I don't think it should matter, but I didn't want to substantially change the current output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WOW!
{ | ||
const RemovePlanAction& action = remove_plan[idx]; | ||
msg::println(msg::format(msgRemovingPackage, msg::spec = action.spec) | ||
.append_fmt_raw(" ({}/{})...", idx + 1, remove_plan.size())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you added append_fmt_raw
now; is this (x/y) notation common enough that we should add an explicit fn for it? (There are at least 2-3 such examples touched in this PR alone but I don't have a barometer on how many others there are)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2-3 are the only ones I've touched recently. I think that refactor can be done separately if/when it's deemed needed.
Hm I dislike that the |
I don’t feel strongly about where that is, only that the relatively useless “starting package” line is byebye |
Having
|
Sure, that can make sense. Given that we didn't do that before this change though I'm not sure what's being seen as the regression? |
Built in implementation of microsoft/vcpkg#23477
Before:
After: