Skip to content
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

Unified object provider backend #911

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
95f0438
Binary cache: async push_success
autoantwort Feb 15, 2023
42ec787
Binary cache: async push_success
autoantwort Feb 15, 2023
0b1d87f
WIP Current state
autoantwort Feb 16, 2023
2d04507
More or less complete implementation
autoantwort Feb 22, 2023
7e1e57e
Supress warnings
autoantwort Feb 22, 2023
e4ed60e
Fix FileObjectProvider
autoantwort Feb 22, 2023
2e71ca8
enable_if 💘
autoantwort Feb 22, 2023
a4200ec
Merge branch 'main' into unified-object-provider-backend
autoantwort Feb 25, 2023
4bbc46a
Clear package dir before unzipping
autoantwort Feb 25, 2023
9d999d8
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Feb 28, 2023
e0ba623
Merge branch 'main' into unified-object-provider-backend
autoantwort Feb 28, 2023
163d9cd
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 2, 2023
2a54205
Apply suggestions from code review
autoantwort Mar 2, 2023
0912655
Adapt code review
autoantwort Mar 2, 2023
5d7288c
Update src/vcpkg/binarycaching.cpp
autoantwort Mar 2, 2023
10189ac
Adapt code review
autoantwort Mar 2, 2023
2567607
Remove unnecessary actions_to_push_notifier.notify_all()
autoantwort Mar 2, 2023
ecdd000
Prevent deadlock and don't be on the crtl+c path
autoantwort Mar 2, 2023
8e7ae61
Add and use BGMessageSink to print IBinaryProvider::push_success mess…
autoantwort Mar 3, 2023
850d7c9
Restore old upload message
autoantwort Mar 3, 2023
548be38
Don't join yourself
autoantwort Mar 4, 2023
6dbbf06
Print messages about remaining packages to upload
autoantwort Mar 4, 2023
74b86fd
Localization
autoantwort Mar 5, 2023
5171d3e
Improve messages
autoantwort Mar 5, 2023
d69ed8f
No singleton and explicit calls to wait_for_async_complete()
autoantwort Mar 5, 2023
2df42d5
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 8, 2023
5f1786e
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 10, 2023
6f5a7ba
Merge branch 'feature/async-binary-cache-push-success' into unified-o…
autoantwort Mar 13, 2023
93303c3
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 16, 2023
8a26c8b
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 19, 2023
aa7e52f
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 22, 2023
d46a4d6
Apply code review
autoantwort Mar 22, 2023
b045c64
Broken: Merge branch 'feature/async-binary-cache-push-success' into u…
autoantwort Mar 22, 2023
104758f
Migrate GHABinaryProvider to ISingleObjectProvider
autoantwort Mar 22, 2023
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
28 changes: 14 additions & 14 deletions azure-pipelines/pipelines.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ jobs:
displayName: 'Rush install, build and test vcpkg-artifacts'
- bash: |
cmake '-DCMAKE_CXX_FLAGS=-fprofile-arcs -ftest-coverage -fPIC -O0 -fsanitize=undefined -fsanitize=address' -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTING=ON -DVCPKG_DEVELOPMENT_WARNINGS=ON -DVCPKG_WARNINGS_AS_ERRORS=ON -DVCPKG_BUILD_BENCHMARKING=ON -DVCPKG_BUILD_FUZZING=ON -B build.amd64.debug
make -j 2 -C build.amd64.debug
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
Comment on lines +47 to +53
Copy link
Member

Choose a reason for hiding this comment

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

Don't think you meant to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

- task: PowerShell@2
displayName: 'Run vcpkg end-to-end tests'
inputs:
Expand Down Expand Up @@ -92,11 +92,11 @@ jobs:
displayName: "Clone vcpkg repo to serve as root"
- bash: |
cmake '-DCMAKE_CXX_FLAGS=-fsanitize=undefined -fsanitize=address' -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTING=ON -DVCPKG_DEVELOPMENT_WARNINGS=ON -DVCPKG_WARNINGS_AS_ERRORS=ON -DVCPKG_BUILD_BENCHMARKING=ON -DVCPKG_BUILD_FUZZING=ON -DCMAKE_OSX_DEPLOYMENT_TARGET=10.13 -B build.amd64.debug
make -j 2 -C build.amd64.debug
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'
#- bash: build.amd64.debug/vcpkg-test
# displayName: 'Run vcpkg tests'
- bash: brew install pkg-config
displayName: 'Install pkgconfig'
- task: PowerShell@2
Expand Down Expand Up @@ -154,7 +154,7 @@ jobs:
call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\Tools\VsDevCmd.bat" -arch=x86 -host_arch=x86
rmdir /s /q build.x86.debug > nul 2> nul
cmake.exe -G Ninja -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTING=ON -DVCPKG_DEVELOPMENT_WARNINGS=ON -DVCPKG_WARNINGS_AS_ERRORS=ON -DVCPKG_BUILD_BENCHMARKING=ON -DVCPKG_BUILD_FUZZING=ON -DVCPKG_BUILD_TLS12_DOWNLOADER=ON -DVCPKG_ARTIFACTS_DEVELOPMENT=ON -B build.x86.debug
ninja.exe -C build.x86.debug all generate-message-map
ninja.exe -C build.x86.debug vcpkg generate-message-map
failOnStderr: true
- task: CodeQL3000Finalize@0
displayName: 'CodeQL Finalize'
Expand All @@ -170,11 +170,11 @@ jobs:
inputs:
PathtoPublish: '$(DiffFile)'
ArtifactName: 'format.diff'
- script: build.x86.debug\vcpkg-test.exe
displayName: "Run vcpkg tests"
failOnStderr: true
env:
VCPKG_ROOT: UNIT_TESTS_SHOULD_NOT_USE_VCPKG_ROOT
#- script: build.x86.debug\vcpkg-test.exe
# displayName: "Run vcpkg tests"
# failOnStderr: true
# env:
# VCPKG_ROOT: UNIT_TESTS_SHOULD_NOT_USE_VCPKG_ROOT
- task: PowerShell@2
displayName: 'Run vcpkg end-to-end tests'
inputs:
Expand Down
27 changes: 12 additions & 15 deletions include/vcpkg/base/downloads.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@

#include <vcpkg/base/fwd/downloads.h>
#include <vcpkg/base/fwd/messages.h>
#include <vcpkg/base/fwd/optional.h>

#include <vcpkg/base/expected.h>
#include <vcpkg/base/files.h>
#include <vcpkg/base/optional.h>
#include <vcpkg/base/span.h>

#include <vcpkg/binarycaching.h>
#include <vcpkg/tools.h>

#include <string>
#include <vector>

Expand Down Expand Up @@ -40,40 +44,33 @@ namespace vcpkg
const Path& file);
std::vector<int> url_heads(View<std::string> urls, View<std::string> headers, View<std::string> secrets);

struct DownloadManagerConfig
{
Optional<std::string> m_read_url_template;
std::vector<std::string> m_read_headers;
Optional<std::string> m_write_url_template;
std::vector<std::string> m_write_headers;
std::vector<std::string> m_secrets;
bool m_block_origin = false;
Optional<std::string> m_script;
};

// Handles downloading and uploading to a content addressable mirror
struct DownloadManager
{
DownloadManager() = default;
explicit DownloadManager(const DownloadManagerConfig& config) : m_config(config) { }
explicit DownloadManager(DownloadManagerConfig&& config) : m_config(std::move(config)) { }

void download_file(Filesystem& fs,
void download_file(Optional<const ToolCache&> tool_cache,
Filesystem& fs,
const std::string& url,
View<std::string> headers,
const Path& download_path,
const Optional<std::string>& sha512,
MessageSink& progress_sink) const;

// Returns url that was successfully downloaded from
std::string download_file(Filesystem& fs,
std::string download_file(Optional<const ToolCache&> tool_cache,
Filesystem& fs,
View<std::string> urls,
View<std::string> headers,
const Path& download_path,
const Optional<std::string>& sha512,
MessageSink& progress_sink) const;

ExpectedL<int> put_file_to_mirror(const Filesystem& fs, const Path& file_to_put, StringView sha512) const;
ExpectedL<int> put_file_to_mirror(Optional<const ToolCache&> tool_cache,
const Filesystem&,
const Path& file_to_put,
StringView sha512) const;

private:
DownloadManagerConfig m_config;
Expand Down
20 changes: 16 additions & 4 deletions include/vcpkg/base/optional.h
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,22 @@ namespace vcpkg
{
constexpr OptionalStorage() noexcept : m_t(nullptr) { }
constexpr OptionalStorage(const T& t) : m_t(&t) { }
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;
template<typename U,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the question is if the following is a valid expressions: T a; independent of the question if it can be constructed or not.

Copy link
Member

@BillyONeal BillyONeal Mar 3, 2023

Choose a reason for hiding this comment

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

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()) { }

std::enable_if_t<std::is_same_v<T, std::decay_t<U>> && !std::is_abstract_v<T>, int*> = nullptr>
constexpr OptionalStorage(const Optional<U>& t) : m_t(t.get())
{
}
template<typename U,
std::enable_if_t<std::is_same_v<T, std::decay_t<U>> && !std::is_abstract_v<T>, int*> = nullptr>
constexpr OptionalStorage(const Optional<const U>& t) : m_t(t.get())
{
}
template<typename U,
std::enable_if_t<std::is_same_v<T, std::decay_t<U>> && !std::is_abstract_v<T>, int*> = nullptr>
constexpr OptionalStorage(Optional<U>&& t) = delete;
template<typename U,
std::enable_if_t<std::is_same_v<T, std::decay_t<U>> && !std::is_abstract_v<T>, int*> = nullptr>
constexpr OptionalStorage(Optional<const U>&& t) = delete;

constexpr bool has_value() const { return m_t != nullptr; }

Expand Down
142 changes: 115 additions & 27 deletions include/vcpkg/binarycaching.h
Original file line number Diff line number Diff line change
@@ -1,18 +1,24 @@
#pragma once

// #include <vcpkg/base/fwd/optional.h>

#include <vcpkg/fwd/binarycaching.h>
#include <vcpkg/fwd/dependencies.h>
#include <vcpkg/fwd/tools.h>
#include <vcpkg/fwd/vcpkgpaths.h>

#include <vcpkg/base/downloads.h>
#include <vcpkg/base/expected.h>
#include <vcpkg/base/files.h>

#include <vcpkg/packagespec.h>
#include <vcpkg/sourceparagraph.h>

#include <condition_variable>
#include <iterator>
#include <queue>
#include <set>
#include <string>
#include <thread>
#include <unordered_map>
#include <vector>

Expand Down Expand Up @@ -42,6 +48,51 @@ namespace vcpkg
const IBinaryProvider* m_available_provider = nullptr; // meaningful iff m_status == available
};

struct BinaryPackageInformation
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

{
explicit BinaryPackageInformation(const InstallPlanAction& action);
std::string package_abi;
// The following fields are only needed for the nuget binary cache
PackageSpec spec;
SourceControlFile& source_control_file;
std::string& raw_version;
std::string compiler_id;
std::string compiler_version;
std::string triplet_abi;
InternalFeatureSet feature_list;
std::vector<PackageSpec> package_dependencies;
};

struct IObjectProvider
{
enum class Access : uint8_t
{
Read = 0b01,
Write = 0b10,
ReadWrite = 0b11,
};
Access access;

bool supports_write() const { return static_cast<uint8_t>(access) & static_cast<uint8_t>(Access::Write); }
bool supports_read() const { return static_cast<uint8_t>(access) & static_cast<uint8_t>(Access::Read); }

IObjectProvider(Access access) : access(access) { }
virtual ~IObjectProvider() = default;

virtual void download(Optional<const ToolCache&> tool_cache,
View<StringView> objects,
const Path& target_dir) const = 0;

virtual void upload(Optional<const ToolCache&> tool_cache,
StringView object_id,
const Path& object_file,
MessageSink& msg_sink) = 0;

virtual void check_availability(Optional<const ToolCache&> tool_cache,
View<StringView> objects,
Span<bool> cache_status) const = 0;
};

struct IBinaryProvider
{
virtual ~IBinaryProvider() = default;
Expand All @@ -52,7 +103,9 @@ namespace vcpkg

/// Called upon a successful build of `action` to store those contents in the binary cache.
/// Prerequisite: action has a package_abi()
virtual void push_success(const InstallPlanAction& action) const = 0;
virtual void push_success(const BinaryPackageInformation& info,
const Path& packages_dir,
MessageSink& msg_sink) = 0;

/// Gives the IBinaryProvider an opportunity to batch any downloading or server communication for
/// executing `actions`.
Expand All @@ -77,61 +130,82 @@ namespace vcpkg
std::vector<std::string> headers_for_get;

LocalizedString valid() const;
std::string instantiate_variables(const InstallPlanAction& action) const;
std::string instantiate_variable(StringView sha) const;

protected:
LocalizedString valid(View<StringLiteral> valid_keys) const;
};

struct BinaryConfigParserState
struct ExtendedUrlTemplate : private UrlTemplate
{
bool nuget_interactive = false;
std::set<StringLiteral> binary_cache_providers;

std::string nugettimeout = "100";

std::vector<Path> archives_to_read;
std::vector<Path> archives_to_write;

std::vector<UrlTemplate> url_templates_to_get;
std::vector<UrlTemplate> url_templates_to_put;
using UrlTemplate::headers_for_get;
using UrlTemplate::headers_for_put;
using UrlTemplate::url_template;
ExtendedUrlTemplate(UrlTemplate&& t) : UrlTemplate(std::move(t)) { }
LocalizedString valid() const;
std::string instantiate_variables(const BinaryPackageInformation& info) const;
};

std::vector<std::string> gcs_read_prefixes;
std::vector<std::string> gcs_write_prefixes;
struct ObjectCacheConfig
{
ObjectCacheConfig() = default;
ObjectCacheConfig(const ObjectCacheConfig&) = delete;
ObjectCacheConfig& operator=(const ObjectCacheConfig&) = delete;
ObjectCacheConfig(ObjectCacheConfig&&) = default;
ObjectCacheConfig& operator=(ObjectCacheConfig&&) = default;
virtual ~ObjectCacheConfig() = default;
std::vector<std::unique_ptr<IObjectProvider>> object_providers;
std::vector<std::string> secrets;
virtual void clear();
};

std::vector<std::string> aws_read_prefixes;
std::vector<std::string> aws_write_prefixes;
bool aws_no_sign_request = false;
struct BinaryConfigParserState : ObjectCacheConfig
{
bool nuget_interactive = false;
std::string nugettimeout = "100";

std::vector<std::string> cos_read_prefixes;
std::vector<std::string> cos_write_prefixes;
std::vector<ExtendedUrlTemplate> url_templates_to_get;
std::vector<ExtendedUrlTemplate> url_templates_to_put;

std::vector<std::string> sources_to_read;
std::vector<std::string> sources_to_write;

std::vector<Path> configs_to_read;
std::vector<Path> configs_to_write;

std::vector<std::string> secrets;
void clear() override;
};

void clear();
struct DownloadManagerConfig : public ObjectCacheConfig
{
bool block_origin = false;
Optional<std::string> script;
void clear() override;
};

ExpectedL<BinaryConfigParserState> create_binary_providers_from_configs_pure(const std::string& env_string,
ExpectedL<BinaryConfigParserState> create_binary_providers_from_configs_pure(Filesystem& fs,
const std::string& env_string,
View<std::string> args);
ExpectedL<std::vector<std::unique_ptr<IBinaryProvider>>> create_binary_providers_from_configs(
const VcpkgPaths& paths, View<std::string> args);

struct BinaryCache
{
BinaryCache() = default;
static BinaryCache* current_instance;
static void wait_for_async_complete();
BinaryCache(Filesystem& filesystem);
explicit BinaryCache(const VcpkgCmdArguments& args, const VcpkgPaths& paths);

~BinaryCache();

void install_providers(std::vector<std::unique_ptr<IBinaryProvider>>&& providers);
void install_providers_for(const VcpkgCmdArguments& args, const VcpkgPaths& paths);

/// Attempts to restore the package referenced by `action` into the packages directory.
RestoreResult try_restore(const InstallPlanAction& action);

/// Called upon a successful build of `action` to store those contents in the binary cache.
void push_success(const InstallPlanAction& action);
void push_success(const InstallPlanAction& action, Path package_dir);

/// Gives the IBinaryProvider an opportunity to batch any downloading or server communication for
/// executing `actions`.
Expand All @@ -143,11 +217,25 @@ namespace vcpkg
std::vector<CacheAvailability> precheck(View<InstallPlanAction> actions);

private:
struct ActionToPush
{
BinaryPackageInformation info;
bool clean_after_push;
Path packages_dir;
};
void push_thread_main();

std::unordered_map<std::string, CacheStatus> m_status;
std::vector<std::unique_ptr<IBinaryProvider>> m_providers;
std::condition_variable actions_to_push_notifier;
std::mutex actions_to_push_mutex;
std::queue<ActionToPush> actions_to_push;
std::thread push_thread;
std::atomic_bool end_push_thread;
Filesystem& filesystem;
};

ExpectedL<DownloadManagerConfig> parse_download_configuration(const Optional<std::string>& arg);
ExpectedL<DownloadManagerConfig> parse_download_configuration(Filesystem& fs, const Optional<std::string>& arg);

std::string generate_nuget_packages_config(const ActionPlan& action);

Expand Down
Loading