In https://github.com/microsoft/vcpkg-tool/pull/1534 I introduced a bug which causes the builtin ports directory to "win" over declared command line --overlay-ports. This was not discovered in testing because the overlay-ports tests don't actually test with names that are also in the builtin ports directory.
Also makes Json::IDeserializer no longer need a virtual dtor.
Before (first one wins):
PS D:\test> type vcpkg.json
{
"dependencies": [
"zlib"
],
"overrides": [{"name": "zlib", "version": "1.2.13"}, {"name": "zlib", "version": "1.3.1"}]
}
Installing 2/2 zlib:x64-windows@1.2.13...
Building zlib:x64-windows@1.2.13...
C:\Users\bion\AppData\Local\vcpkg\registries\git-trees\ad5a49006f73b45b715299515f31164131b51982: info: installing overlay port from here
-- Using cached madler-zlib-v1.2.13.tar.gz.
-- Extracting source D:/vcpkg-downloads/madler-zlib-v1.2.13.tar.gz
-- Applying patch 0001-Prevent-invalid-inclusions-when-HAVE_-is-set-to-0.patch
-- Applying patch 0002-skip-building-examples.patch
-- Applying patch 0003-build-static-or-shared-not-both.patch
-- Applying patch 0004-android-and-mingw-fixes.patch
-- Using source at D:/vcpkg/buildtrees/zlib/src/v1.2.13-f30d2a168d.clean
-- Found external ninja('1.12.1').
-- Configuring x64-windows
-- Building x64-windows-dbg
-- Building x64-windows-rel
-- Installing: D:/vcpkg/packages/zlib_x64-windows/share/zlib/vcpkg-cmake-wrapper.cmake
-- Fixing pkgconfig file: D:/vcpkg/packages/zlib_x64-windows/lib/pkgconfig/zlib.pc
-- Using cached msys2-mingw-w64-x86_64-pkgconf-1~2.3.0-1-any.pkg.tar.zst.
-- Using cached msys2-msys2-runtime-3.5.4-2-x86_64.pkg.tar.zst.
-- Using msys root at D:/vcpkg-downloads/tools/msys2/21caed2f81ec917b
-- Fixing pkgconfig file: D:/vcpkg/packages/zlib_x64-windows/debug/lib/pkgconfig/zlib.pc
-- Installing: D:/vcpkg/packages/zlib_x64-windows/share/zlib/copyright
-- Performing post-build validation
Stored binaries in 1 destinations in 93.9 ms.
Elapsed time to handle zlib:x64-windows: 3.7 s
zlib:x64-windows package ABI: a858cb84557b70b9fa3b3934233ce9667bef3fcf11c99b00070f799cc44bff14
Total install time: 3.7 s
The package zlib is compatible with built-in CMake targets:
find_package(ZLIB REQUIRED)
target_link_libraries(main PRIVATE ZLIB::ZLIB)
After: (an error)
D:\test\vcpkg.json: error: $.overrides[1] (an override): zlib already has a declared override
note: Extended documentation available at 'https://learn.microsoft.com/vcpkg/users/manifests?WT.mc_id=vcpkg_inproduct_cli'.
Resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1665249
Content-Range is 0 indexed, although nobody seemed to have encountered
any issues as a result of this on github.com, but on GitHub Enterprise
server it resulted in a InvalidChunkException error
Extensive overhaul of our downloads handling and console output; @JavierMatosD and I have gone back and forth several times and yet kept introducing unintended bugs in other places, which led me to believe targeted fixes would no longer cut it.
Fixes many longstanding bugs and hopefully makes our console output for this more understandable:
* We no longer print 'error' when an asset cache misses but the authoritative download succeeds. This partially undoes #1541. It is good to print errors immediately when they happen, but if a subsequent authoritative download succeeds we need to not print those errors.
* We now always and consistently print output from x-script s at the time that actually happens. Resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2300063
* We don't tell the user that proxy settings might fix a hash mismatch problem.
* We do tell the user that proxy settings might fix a download from asset cache problem.
* We now always tell the user the full command line we tried when invoking an x-script that fails.
* We don't crash if an x-script doesn't create the file we expect, or creates a file with the wrong hash.
* We now always print what we are doing *before* touching the network, so if we hang the user knows which server is being problematic. Note that this includes storing back to asset caches which we were previously entirely silent about except in case of failure.
Other changes:
* Removed debug output about asset cache configuration. The output was misleading / wrong depending on readwrite settings, and echoing to the user exactly what they said before we've interpreted it is not useful debug output. (Contrast with other `VcpkgPaths` debug output which tend to be paths we have likely changed from something a user said)
Other notes:
* This makes all dependencies of #908 speak `DiagnosticContext` so it will be easy to audit that the foreground/background thread behavior is correct after this.
* I did test the curl status parsing on old Ubuntu again.
Special thanks to @JavierMatosD for his help in review of the first console output attempts and for blowing the dust off this area in the first place.
Other notable behavior changes:
* Attempting to set `overlay-ports` to the provably same directory as a top-level manifest now generates an error, with a special error for `"."` in the configuration file. Note that `overlay-triplets` are *not* affected.
* Previously, the `load_all_control_files` family on `OverlayPortProviderImpl` would load all ports in the lowest priority `--overlay-ports`, then overwrite any of those with 'higher' values. This meant that malformed ports which vcpkg would never consider would be forced to be loaded, thus potentially triggering irrelevant errors.
* Previously, some paths that considered overlay ports in the "subdirectories of named overlay ports" would enforce that their contained `CONTROL`/`vcpkg.json` declared a matching name, and some paths did not. Now, all paths consider ports with mismatched names an error rather than silently using the name in `vcpkg.json`.
* Previously, there were additional `fs.exists` checks in order to determine if the indicated directory should contain the port or subdirectories that are ports; now, we `try_read_contents` and consider file not found as nonexistent, saving a disk operation.
* `commands.install.cpp` no longer gets entirely broken in the 'add the builtin ports directory as an overlay' cases if there is a `vcpkg.json` in `%VCPKG_ROOT%/ports`.
Related:
* https://github.com/microsoft/vcpkg-docs/pull/422 (Documentation update clarifying overlay-ports)
Resolves:
* https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1979597
A large customer took a dependency on our happening to use lowercase SHA512s most of the time, and were broken when recent changes to vcpkgTools.xml contained capitalized SHA512s. This change always lowercases the SHA before invoking the x-script.
* Fix https://github.com/microsoft/vcpkg/issues/32522
Currently, the `download_files` function is only used by binary caching providers (`HttpGetBinaryProvider` and `GHABinaryProvider`), so I think it is okay to change the function so that it does not exit with error.
I manually tested following scenarios:
| **--only-binarycaching** | **server up** | **server down** |
| ------------------------ | ------------- | ------------------ |
| **Without** | OK | Continue with Warn |
| **With** | OK | Exit with Error |
* Move retries for curl operations into curl itself.
* Added function to parse curl status lines with error messages embedded and tests.
* Add getting exit code and error message for each curl operation, and return those operation results through ExpectedL<int> rather than terminating vcpkg.
* Add test case handling for old curl tested on Ubuntu 20.04.
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
The lambda 'find_command' was potentially confusable with the function that implements `vcpkg find`. Moreover, the capture of the whole VcpkgCmdArguments when only the desired command name was needed was overkill. The simple for loop is shorter, easier to debug, and avoids needing to form the complicated 'decltype' to return nullptr.
* In several PRs, such as https://github.com/microsoft/vcpkg-tool/pull/908 and https://github.com/microsoft/vcpkg-tool/pull/1210 , it has become clear that we need a thread safe channel through which errors and warnings can both be reasonably reported. Now that https://github.com/microsoft/vcpkg-tool/pull/1279 is landed and functionally everything in the codebase uses ExpectedL, we can look at what the new thing that fixes issues is.
Consider the following:
```c++
ExpectedL<T> example_api(int a);
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port_manifest_text(StringView text,
StringView control_path,
MessageSink& warning_sink);
```
The reason this can't return the warnings through the ExpectedL channel is that we don't want the 'error' state to be engaged when there are merely warnings. Moreover, that these channels are different channels means that situations that might want to return errors and warnings together, as happens when parsing files, means that order relationships between errors and warnings is lost. It is probably a good idea in general to put warnings and errors about the same location next to each other in the output, but that's hard to do with this interface.
Rather than multiplexing everything through the return value, this proposal is to multiplex only the success or failure through the return value, and report any specific error information through an out parameter.
1. Distinguish whether an overall operation succeeded or failed in the return value, but
2. record any errors or warnings via an out parameter.
Applying this to the above gives:
```c++
Optional<T> example_api(MessageContext& context, int a);
// unique_ptr is already 'optional'
std::unique_ptr<SourceControlFile> try_load_port_manifest_text(MessageContext& context,
StringView text,
StringView control_path);
```
Issues this new mechanism fixes:
* Errors and warnings can share the same channel and thus be printed together
* The interface between code wanting to report events and the code wanting to consume them is a natural thread synchronization boundary. Other attempts to fix this have been incorrect by synchronizing individual print calls ( https://github.com/microsoft/vcpkg-tool/pull/1290 ) or complex enough that we are not sure they are correct by trying to recover boundaries by reparsing our own error output ( https://github.com/microsoft/vcpkg-tool/pull/908 )
* This shuts down the "error: error:" and similar bugs where it isn't clear who is formatting the overall error message vs. talking about individual components
Known issues that are not fixed by this change:
* This still doesn't make it easy for callers to programmatically handle specific types of errors. Currently, we have some APIs that still use explicit `std::error_code` because they want to do different things for 'file does not exist' vs. 'there was an I/O error'. Given that this condition isn't well served by the ExpectedL mechanism I don't want to wait until we have a better solution to it to proceed.
* Because we aren't making the context parameter the 'success carrier' it's more complex to implement 'warnings as errors' or similar functionality where the caller decides how 'important' something is. I would be in favor of moving all success tests to the context parameter but I'm not proposing that because the other vcpkg maintainers do not like it.
* Contextual information / stack problems aren't solved. However, the context parameter might be extended in the future to help with this.
Billy O'Neal
Embedded Bot
autoantwort
CSIGS@microsoft.com
Thomas1664
Javier Matos Denizac
Jamie
Alexander Neumann
Victor Romero
Seunghyun Hwang
WangWeiLin-MV
PARK DongHa