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.

Sign up for GitHub

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

Jump to bottom

Fix the Android prefab export. #1344

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Fix the Android prefab export. #1344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5fd5d5c
Skip the emscripten cmake script execution.
dmn-star Feb 10, 2024
5c4df5e
Host-only ports (e.g. vcpkg_cmake) are not to be exported.
dmn-star Feb 10, 2024
e74b1a7
The port version is now ignored when looking for *.list files.
dmn-star Feb 10, 2024
0efad5e
Reformat code and silence the clang-tidy.
dmn-star Feb 10, 2024
6cb950b
Reformat the sources again.
dmn-star Feb 10, 2024
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
3 changes: 2 additions & 1 deletion include/vcpkg/export.prefab.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ namespace vcpkg::Prefab
void do_export(const std::vector<ExportPlanAction>& export_plan,
const VcpkgPaths& paths,
const Options& prefab_options,
const Triplet& triplet);
const Triplet& triplet,
const Triplet& host_triplet);
Optional<StringView> find_ndk_version(StringView content);
Optional<NdkVersion> to_version(StringView version);
}
2 changes: 1 addition & 1 deletion src/vcpkg/commands.export.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ namespace vcpkg

if (opts.prefab)
{
Prefab::do_export(export_plan, paths, opts.prefab_options, default_triplet);
Prefab::do_export(export_plan, paths, opts.prefab_options, default_triplet, host_triplet);
}

Checks::exit_success(VCPKG_LINE_INFO);
Expand Down
32 changes: 23 additions & 9 deletions src/vcpkg/export.prefab.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ namespace vcpkg::Prefab
static std::string null_if_empty(const std::string& str)
{
std::string copy = str;
if (copy.size() == 0)
if (copy.empty())
{
copy = "null";
}
Expand All @@ -80,7 +80,7 @@ namespace vcpkg::Prefab
static std::string null_if_empty_array(const std::string& str)
{
std::string copy = str;
if (copy.size() == 0)
if (copy.empty())
{
copy = "null";
}
Expand Down Expand Up @@ -230,7 +230,7 @@ namespace vcpkg::Prefab
settings.environment = get_clean_environment();
const int exit_code = cmd_execute(cmd, settings).value_or_exit(VCPKG_LINE_INFO);

if (!(exit_code == 0))
if (exit_code != 0)
{
msg::println_error(msgInstallingMavenFile, msg::path = aar);
Checks::exit_fail(VCPKG_LINE_INFO);
Expand All @@ -253,7 +253,8 @@ namespace vcpkg::Prefab
void do_export(const std::vector<ExportPlanAction>& export_plan,
const VcpkgPaths& paths,
const Options& prefab_options,
const Triplet& default_triplet)
const Triplet& default_triplet,
const Triplet& host_triplet)
{
auto provider = CMakeVars::make_triplet_cmake_var_provider(paths);

Expand Down Expand Up @@ -281,8 +282,12 @@ namespace vcpkg::Prefab

for (const auto& triplet_file : triplet_db.available_triplets)
{
if (triplet_file.name.size() > 0)
if (!triplet_file.name.empty())
{
// The execution of emscripten cmake script causes the prefab export to fail.
// But here we don't need this execution at all, so we skip it.
if (triplet_file.name == "wasm32-emscripten") continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this isn't the right direction. Triplet names don't carry much meaning. They are identifiers.

Basically, it shouldn't care about any triplet which isn't installed.
Looking at the past issues, I think what is really desired is a user provided positive list of triplets, so that you can pick some architectures.

Copy link
Author

Choose a reason for hiding this comment

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

The fix is quite simple, yet it will repair the prefab export, which has been broken for 4 (?) years.

When I think about the command line, I would just omit the triplets entirely.

vcpkg export prefab

or or if you just want to export a specific ABI

vcpkg export prefab  --ABI arm64-v8a

Copy link
Member

Choose a reason for hiding this comment

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

which has been broken for 4 (?) years.

A broken fix is not made correct because the bug attempting to be fixed is old.

I agree with @dg0yt that you can't assume anything based on the triplet name. I can make my identical-to-wasm32-emscripten triplet named "fish".


Triplet triplet = Triplet::from_canonical_name(triplet_file.name);
auto triplet_build_info = build_info_from_triplet(paths, provider, triplet);
if (is_supported(*triplet_build_info))
Expand Down Expand Up @@ -372,6 +377,10 @@ namespace vcpkg::Prefab

for (const auto& action : export_plan)
{
// cross-compiling
// Host-only ports (e.g. vcpkg_cmake) are not to be exported.
if (host_triplet == action.spec.triplet()) continue;

const std::string name = action.spec.name();
auto dependencies = action.dependencies();

Expand Down Expand Up @@ -400,7 +409,10 @@ namespace vcpkg::Prefab
const auto per_package_dir_path = paths.prefab / name;

const auto& binary_paragraph = action.core_paragraph().value_or_exit(VCPKG_LINE_INFO);
const std::string norm_version = binary_paragraph.version.to_string();

// The port version is not specified during installation (vcpkg install). Just ignore port version.
// jsoncpp_1.17#2_x64-android.list -> jsoncpp_1.17_x64-android.list
const std::string norm_version = binary_paragraph.version.text;

version_map[name] = norm_version;

Expand Down Expand Up @@ -464,7 +476,7 @@ namespace vcpkg::Prefab

std::vector<std::string> pom_dependencies;

if (dependencies_minus_empty_packages.size() > 0)
if (!dependencies_minus_empty_packages.empty())
{
pom_dependencies.emplace_back("\n<dependencies>");
}
Expand All @@ -485,7 +497,7 @@ namespace vcpkg::Prefab
pm.dependencies.push_back(it.name());
}

if (dependencies_minus_empty_packages.size() > 0)
if (!dependencies_minus_empty_packages.empty())
{
pom_dependencies.emplace_back("</dependencies>\n");
}
Expand All @@ -509,7 +521,7 @@ namespace vcpkg::Prefab
}

Debug::print(
fmt::format("Found {} triplets:\n\t{}", triplets.size(), Strings::join("\n\t", triplet_names)));
fmt::format("Found {} triplets:\n\t{}\n", triplets.size(), Strings::join("\n\t", triplet_names)));

for (const auto& triplet : triplets)
{
Expand All @@ -519,6 +531,8 @@ namespace vcpkg::Prefab
if (!(fs.exists(listfile, IgnoreErrors{})))
{
msg::println_error(msgCorruptedInstallTree);
msg::println_error(msgFileNotFound, msg::path = listfile);

Checks::exit_fail(VCPKG_LINE_INFO);
}

Expand Down
Loading