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

[vcpkg] Merge the vcpkg metadata uploader into the vcpkg binary #13421

Merged
merged 11 commits into from
Sep 11, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
13 changes: 1 addition & 12 deletions docs/tool-maintainers/layout.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,6 @@ We have six files in this directory -- one `.clang-format` file, one
the IDE.
- `vcpkg.sln`: The solution file is how one opens the project in the VS IDE.

### `vcpkg`, `vcpkglib`, `vcpkgmetricsuploader`, and `vcpkgtest`

These four contain exactly one `<name>.vcxproj` and one
`<name>.vcxproj.filters`. The `<name>.vcxproj` file contains the source files
and the `<name>.vcxproj.filters` contains information on how Visual Studio
should lay out the project's source files in the IDE's project view.

`vcpkgtest` should not be touched. It's likely that it will be deleted soon. If
you want to test your code, use the cmake build system.

## Source Files

If you're modifying the project, it's likely that these are the directories that
Expand All @@ -72,8 +62,7 @@ There are three directories:
### `src`

The source files live here. `pch.cpp` is the source file for the
[precompiled header]; `vcpkg.cpp` is where the `vcpkg` binary lives; and
`vcpkgmetricsuploader.cpp` is where the metrics uploader lives.
[precompiled header]; `vcpkg.cpp` is where the `vcpkg` binary lives.

The interesting files live in the `vcpkg` and `vcpkg-test` directories. In
`vcpkg`, you have the implementation for the interfaces that live in
Expand Down
7 changes: 1 addition & 6 deletions scripts/bootstrap.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ $arguments = (
"/verbosity:minimal",
"/m",
"/nologo",
"`"$vcpkgBootstrapPath\dirs.proj`"") -join " "
"`"$vcpkgBootstrapPath\vcpkg.vcxproj`"") -join " "

function vcpkgInvokeCommandClean()
{
Expand Down Expand Up @@ -432,9 +432,4 @@ Write-Verbose "Placing vcpkg.exe in the correct location"

Copy-Item "$vcpkgReleaseDir\vcpkg.exe" "$vcpkgRootDir\vcpkg.exe"

if (-not $disableMetrics)
{
Copy-Item "$vcpkgReleaseDir\vcpkgmetricsuploader.exe" "$vcpkgRootDir\scripts\vcpkgmetricsuploader.exe"
}

Remove-Item "$vcpkgReleaseDir" -Force -Recurse -ErrorAction SilentlyContinue
11 changes: 0 additions & 11 deletions toolsrc/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ file(GLOB VCPKGLIB_BASE_INCLUDES CONFIGURE_DEPENDS include/vcpkg/base/*.h includ

set(VCPKG_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/src/vcpkg.cpp)

set(VCPKGMETRICSUPLOADER_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/src/vcpkgmetricsuploader.cpp)

file(GLOB VCPKG_TEST_SOURCES CONFIGURE_DEPENDS src/vcpkg-test/*.cpp)
file(GLOB VCPKG_TEST_INCLUDES CONFIGURE_DEPENDS include/vcpkg-test/*.h)

Expand Down Expand Up @@ -116,14 +114,6 @@ add_executable(vcpkg ${VCPKG_SOURCES})
target_link_libraries(vcpkg PRIVATE vcpkglib)
vcpkg_target_add_warning_options(vcpkg)

# === Target: vcpkgmetricsuploader ===

if(WIN32 AND NOT VCPKG_DISABLE_METRICS)
add_executable(vcpkgmetricsuploader WIN32 ${VCPKGMETRICSUPLOADER_SOURCES})
target_link_libraries(vcpkgmetricsuploader PRIVATE vcpkglib)
vcpkg_target_add_warning_options(vcpkgmetricsuploader)
endif()

# === Target: vcpkg-test ===

if (BUILD_TESTING)
Expand Down Expand Up @@ -164,7 +154,6 @@ if(CLANG_FORMAT)
COMMAND ${CLANG_FORMAT} -i -verbose ${VCPKGLIB_INCLUDES}

COMMAND ${CLANG_FORMAT} -i -verbose ${VCPKG_SOURCES}
COMMAND ${CLANG_FORMAT} -i -verbose ${VCPKGMETRICSUPLOADER_SOURCES}

COMMAND ${CLANG_FORMAT} -i -verbose ${VCPKG_TEST_SOURCES}
COMMAND ${CLANG_FORMAT} -i -verbose ${VCPKG_TEST_INCLUDES}
Expand Down
22 changes: 22 additions & 0 deletions toolsrc/include/vcpkg/commands.upload-metrics.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#pragma once

#if !VCPKG_DISABLE_METRICS && defined(_WIN32)
#define VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND 1
#else
#define VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND 0
#endif // !VCPKG_DISABLE_METRICS && defined(_WIN32)

#if VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND

#include <vcpkg/commands.interface.h>

namespace vcpkg::Commands::UploadMetrics
{
extern const CommandStructure COMMAND_STRUCTURE;
struct UploadMetricsCommand : BasicCommand
{
virtual void perform_and_exit(const VcpkgCmdArguments& args, Files::Filesystem& fs) const override;
};
}

#endif // VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND
108 changes: 60 additions & 48 deletions toolsrc/src/vcpkg-test/commands.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,68 +2,80 @@

#include <vcpkg/commands.contact.h>
#include <vcpkg/commands.h>
#include <vcpkg/commands.upload-metrics.h>
#include <vcpkg/commands.version.h>

#include <stddef.h>

using namespace vcpkg;

TEST_CASE ("test commands are constructible", "[commands]")
namespace
{
Commands::Contact::ContactCommand contact{};
Commands::Version::VersionCommand version{};
}
template<class CommandListT, size_t ExpectedCount>
void check_all_commands(const CommandListT& actual_commands, const char* const (&expected_commands)[ExpectedCount])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: take std::initializer_list<StringLiteral> as the second argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that's an improvement: initializer_list exists to be the thing that controls constructor overload resolution; using it for any other purpose just encourages use after free bugs when people treat it like a container instead of a pointer to a temporary array.

{
CHECK(actual_commands.size() == ExpectedCount); // makes sure this test is updated if we add a command
for (const char* expected_command : expected_commands)
{
CHECK(Commands::find(StringView{expected_command, strlen(expected_command)}, actual_commands) != nullptr);
}

CHECK(Commands::find("x-never-will-exist", actual_commands) == nullptr);
}
} // unnamed namespace

// clang-format tries to wrap the following lists inappropriately

// clang-format off
TEST_CASE ("get_available_basic_commands works", "[commands]")
{
auto commands_list = Commands::get_available_basic_commands();
CHECK(commands_list.size() == 2);
CHECK(Commands::find("version", commands_list) != nullptr);
CHECK(Commands::find("contact", commands_list) != nullptr);
CHECK(Commands::find("aang", commands_list) == nullptr);
check_all_commands(Commands::get_available_basic_commands(), {
"contact",
"version",
#if VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND
"x-upload-metrics",
#endif // VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND
});
}

TEST_CASE ("get_available_paths_commands works", "[commands]")
{
auto commands_list = Commands::get_available_paths_commands();
CHECK(commands_list.size() == 19);

CHECK(Commands::find("/?", commands_list) != nullptr);
CHECK(Commands::find("help", commands_list) != nullptr);
CHECK(Commands::find("search", commands_list) != nullptr);
CHECK(Commands::find("list", commands_list) != nullptr);
CHECK(Commands::find("integrate", commands_list) != nullptr);
CHECK(Commands::find("owns", commands_list) != nullptr);
CHECK(Commands::find("update", commands_list) != nullptr);
CHECK(Commands::find("edit", commands_list) != nullptr);
CHECK(Commands::find("create", commands_list) != nullptr);
CHECK(Commands::find("cache", commands_list) != nullptr);
CHECK(Commands::find("portsdiff", commands_list) != nullptr);
CHECK(Commands::find("autocomplete", commands_list) != nullptr);
CHECK(Commands::find("hash", commands_list) != nullptr);
CHECK(Commands::find("fetch", commands_list) != nullptr);
CHECK(Commands::find("x-ci-clean", commands_list) != nullptr);
CHECK(Commands::find("x-history", commands_list) != nullptr);
CHECK(Commands::find("x-package-info", commands_list) != nullptr);
CHECK(Commands::find("x-vsinstances", commands_list) != nullptr);
CHECK(Commands::find("x-format-manifest", commands_list) != nullptr);

CHECK(Commands::find("korra", commands_list) == nullptr);
check_all_commands(Commands::get_available_paths_commands(), {
"/?",
"help",
"search",
"list",
"integrate",
"owns",
"update",
"edit",
"create",
"cache",
"portsdiff",
"autocomplete",
"hash",
"fetch",
"x-ci-clean",
"x-history",
"x-package-info",
"x-vsinstances",
"x-format-manifest",
});
}

TEST_CASE ("get_available_commands_type_a works", "[commands]")
{
auto commands_list = Commands::get_available_triplet_commands();
CHECK(commands_list.size() == 10);

CHECK(Commands::find("install", commands_list) != nullptr);
CHECK(Commands::find("x-set-installed", commands_list) != nullptr);
CHECK(Commands::find("ci", commands_list) != nullptr);
CHECK(Commands::find("remove", commands_list) != nullptr);
CHECK(Commands::find("upgrade", commands_list) != nullptr);
CHECK(Commands::find("build", commands_list) != nullptr);
CHECK(Commands::find("env", commands_list) != nullptr);
CHECK(Commands::find("build-external", commands_list) != nullptr);
CHECK(Commands::find("export", commands_list) != nullptr);
CHECK(Commands::find("depend-info", commands_list) != nullptr);

CHECK(Commands::find("mai", commands_list) == nullptr);
check_all_commands(Commands::get_available_triplet_commands(), {
"install",
"x-set-installed",
"ci",
"remove",
"upgrade",
"build",
"env",
"build-external",
"export",
"depend-info",
});
}
// clang-format on
5 changes: 4 additions & 1 deletion toolsrc/src/vcpkg/base/system.process.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,8 @@ namespace vcpkg
STARTUPINFOW startup_info;
memset(&startup_info, 0, sizeof(STARTUPINFOW));
startup_info.cb = sizeof(STARTUPINFOW);
startup_info.dwFlags = STARTF_USESHOWWINDOW;
startup_info.wShowWindow = SW_HIDE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we always want these flags?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we do but the bits could be renamed to be clearer, will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does my rename resolve your concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

Very much, thanks!


return windows_create_process(cmd_line, env, dwCreationFlags, startup_info);
}
Expand Down Expand Up @@ -547,7 +549,8 @@ namespace vcpkg
{
auto timer = Chrono::ElapsedTimer::create_started();

auto process_info = windows_create_process(cmd_line, {}, DETACHED_PROCESS | CREATE_BREAKAWAY_FROM_JOB);
auto process_info =
windows_create_process(cmd_line, {}, CREATE_NEW_CONSOLE | CREATE_NO_WINDOW | CREATE_BREAKAWAY_FROM_JOB);
if (!process_info.get())
{
Debug::print("cmd_execute_no_wait() failed with error code ", process_info.error(), "\n");
Expand Down
8 changes: 8 additions & 0 deletions toolsrc/src/vcpkg/commands.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <vcpkg/commands.search.h>
#include <vcpkg/commands.setinstalled.h>
#include <vcpkg/commands.upgrade.h>
#include <vcpkg/commands.upload-metrics.h>
#include <vcpkg/commands.version.h>
#include <vcpkg/commands.xvsinstances.h>
#include <vcpkg/export.h>
Expand All @@ -38,9 +39,16 @@ namespace vcpkg::Commands
{
static const Version::VersionCommand version{};
static const Contact::ContactCommand contact{};
#if VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND
static const UploadMetrics::UploadMetricsCommand upload_metrics{};
#endif // VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND

static std::vector<PackageNameAndFunction<const BasicCommand*>> t = {
{"version", &version},
{"contact", &contact},
#if VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND
{"x-upload-metrics", &upload_metrics},
#endif // VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND
};
return t;
}
Expand Down
29 changes: 29 additions & 0 deletions toolsrc/src/vcpkg/commands.upload-metrics.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#include <vcpkg/commands.upload-metrics.h>

#if VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND
#include <vcpkg/base/checks.h>
BillyONeal marked this conversation as resolved.
Show resolved Hide resolved
#include <vcpkg/base/files.h>

#include <vcpkg/metrics.h>
#include <vcpkg/vcpkgcmdarguments.h>

using namespace vcpkg;

namespace vcpkg::Commands::UploadMetrics
{
const CommandStructure COMMAND_STRUCTURE = {
create_example_string("x-upload-metrics metrics.txt"),
1,
1,
};

void UploadMetricsCommand::perform_and_exit(const VcpkgCmdArguments& args, Files::Filesystem& fs) const
{
(void)args.parse_arguments(COMMAND_STRUCTURE);
const auto& payload_path = args.command_arguments[0];
auto payload = fs.read_contents(payload_path).value_or_exit(VCPKG_LINE_INFO);
Metrics::g_metrics.lock()->upload(payload);
Checks::exit_success(VCPKG_LINE_INFO);
}
}
#endif // VCPKG_ENABLE_X_UPLOAD_METRICS_COMMAND
26 changes: 6 additions & 20 deletions toolsrc/src/vcpkg/metrics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -491,41 +491,27 @@ namespace vcpkg::Metrics

const fs::path temp_folder_path = fs::path(temp_folder) / "vcpkg";
const fs::path temp_folder_path_exe =
temp_folder_path / Strings::format("vcpkgmetricsuploader-%s.exe", Commands::Version::base_version());
temp_folder_path / Strings::format("vcpkg-%s.exe", Commands::Version::base_version());
#endif

#if defined(_WIN32)

const fs::path exe_path = [&fs]() -> fs::path {
auto vcpkgdir = System::get_exe_path_of_current_process().parent_path();
auto path = vcpkgdir / "vcpkgmetricsuploader.exe";
if (fs.exists(path)) return path;

path = vcpkgdir / "scripts" / "vcpkgmetricsuploader.exe";
if (fs.exists(path)) return path;

return "";
}();

std::error_code ec;
#if defined(_WIN32)
fs.create_directories(temp_folder_path, ec);
if (ec) return;
fs.copy_file(exe_path, temp_folder_path_exe, fs::copy_options::skip_existing, ec);
fs.copy_file(
System::get_exe_path_of_current_process(), temp_folder_path_exe, fs::copy_options::skip_existing, ec);
if (ec) return;
#else
if (!fs.exists("/tmp")) return;
const fs::path temp_folder_path = "/tmp/vcpkg";
std::error_code ec;
fs.create_directory(temp_folder_path, ec);
// ignore error
ec.clear();
fs.create_directory(temp_folder_path, ignore_errors);
#endif
const fs::path vcpkg_metrics_txt_path = temp_folder_path / ("vcpkg" + generate_random_UUID() + ".txt");
fs.write_contents(vcpkg_metrics_txt_path, payload, ec);
if (ec) return;

#if defined(_WIN32)
const std::string cmd_line = Strings::format("cmd /c \"start \"vcpkgmetricsuploader.exe\" \"%s\" \"%s\"\"",
const std::string cmd_line = Strings::format("\"%s\" x-upload-metrics \"%s\"\"",
BillyONeal marked this conversation as resolved.
Show resolved Hide resolved
fs::u8string(temp_folder_path_exe),
fs::u8string(vcpkg_metrics_txt_path));
System::cmd_execute_no_wait(cmd_line);
Expand Down
22 changes: 0 additions & 22 deletions toolsrc/src/vcpkgmetricsuploader.cpp

This file was deleted.

20 changes: 0 additions & 20 deletions toolsrc/windows-bootstrap/dirs.proj

This file was deleted.

Loading