Unified object provider backend#911
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
autoantwort
force-pushed
the
unified-object-provider-backend
branch
from
033a06c to
2d04507
Compare
|
@vicroms This is now more or less done |
# Conflicts: # include/vcpkg/binarycaching.h # src/vcpkg/binarycaching.cpp
# Conflicts: # src/vcpkg.cpp
# Conflicts: # src/vcpkg.cpp
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
| constexpr OptionalStorage(const Optional<const T>& t) : m_t(t.get()) { } | ||
| constexpr OptionalStorage(Optional<T>&& t) = delete; | ||
| constexpr OptionalStorage(Optional<const T>&& t) = delete; | ||
| template<typename U, |
There was a problem hiding this comment.
Can you explain why this change was necessary? Optional stores T, not T*, so whether T is abstract should not matter. (is_abstract is almost never the right trait; usually people are looking for is_constructible or similar....)
There was a problem hiding this comment.
From Discord:
the problem is OptionalStorage<const U&, B> wanting a Optional<U> and U = Interface due to deduction
(https://godbolt.org/z/4Gf4e7PKT uncomment line 295 for the error)
There was a problem hiding this comment.
I also don't see how you would use is_constructible here since the input args are unknown. U might be constructible given the correct args (which we don't know given the context) however if U is an abstract class Optional<U> can simply not exist.
There was a problem hiding this comment.
So the question is if the following is a valid expressions: T a; independent of the question if it can be constructed or not.
There was a problem hiding this comment.
Oh, I see, the problem is that given Optional<const T&> we try to form Optional<T> due to:
template<class T, bool B>
struct OptionalStorage<const T&, B>
{
// T doesn't have the 'const&' here
constexpr OptionalStorage(const Optional<T>& t) : m_t(t.get()) { }
};
I believe the correct fix is just to put the const& back on rather than this SFINAE. That is, in struct OptionalStorage<T&, B>,
constexpr OptionalStorage(Optional<T>& t) : m_t(t.get()) { }
should be
constexpr OptionalStorage(Optional<T&>& t) : m_t(t.get()) { }
and in struct OptionalStorage<const T&, B>,
constexpr OptionalStorage(const Optional<T>& t) : m_t(t.get()) { }
constexpr OptionalStorage(const Optional<const T>& t) : m_t(t.get()) { }
constexpr OptionalStorage(Optional<T>&& t) = delete;
constexpr OptionalStorage(Optional<const T>&& t) = delete;
should just be
constexpr OptionalStorage(const Optional<const T&>& t) : m_t(t.get()) { }
| const IBinaryProvider* m_available_provider = nullptr; // meaningful iff m_status == available | ||
| }; | ||
|
|
||
| struct BinaryPackageInformation |
There was a problem hiding this comment.
Hmmm I wonder if this scheme makes it easier to pass around shared cache information, like the zip, between config providers so we can get out of the business of multiple destinations trying to share the same provider?
There was a problem hiding this comment.
Yeah this is the idea behind this PR and implemented. The IObjectProvider's only upload and download objects (in this case the zips, but they also can be used for asset caching). Since the zipping is now not done inside the IObjectProvider's they now represent one destination.
| make -j 2 -C build.amd64.debug vcpkg | ||
| displayName: "Build vcpkg with CMake" | ||
| failOnStderr: true | ||
| - bash: build.amd64.debug/vcpkg-test | ||
| displayName: 'Run vcpkg tests' | ||
| env: | ||
| VCPKG_ROOT: UNIT_TESTS_SHOULD_NOT_USE_VCPKG_ROOT | ||
| #- bash: build.amd64.debug/vcpkg-test | ||
| # displayName: 'Run vcpkg tests' | ||
| # env: | ||
| # VCPKG_ROOT: UNIT_TESTS_SHOULD_NOT_USE_VCPKG_ROOT |
There was a problem hiding this comment.
Don't think you meant to do that?
There was a problem hiding this comment.
The tests do not build currently (expected) but I want to run the e2e tests in the ci, so I have to disable the unit tests temporarily.
…ages between package installs Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
# Conflicts: # src/vcpkg/build.cpp
# Conflicts: # src/vcpkg/base/messages.cpp
…bject-provider-backend # Conflicts: # include/vcpkg/binarycaching.h # include/vcpkg/fwd/binarycaching.h # src/vcpkg-test/binarycaching.cpp # src/vcpkg/binarycaching.cpp # src/vcpkg/build.cpp # src/vcpkg/configure-environment.cpp # src/vcpkg/install.cpp
# Conflicts: # include/vcpkg/base/messages.h # src/vcpkg/base/messages.cpp
…nified-object-provider-backend # Conflicts: # src/vcpkg/binarycaching.cpp # src/vcpkg/commands.xdownload.cpp
3 participants
cc @vicroms
Far from done, but the main point is to have a new
IObjectProviderinterface that can be used by the asset and the binary cache. For the unified options a newConfigSegmentsParsershould be created that have the shared options of asset and binary caching. The parsers for asset and binary caching can then inherit from this new parser base.PS: Only this changes are interesting, the others are from #908