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

More expressive checks in triplet/port #1379

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

Conversation

Thomas1664
Copy link
Contributor

This makes it more clear what is actually checked here.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

The changes other than is_overlay_port look good to me.

include/vcpkg/sourceparagraph.h Outdated Show resolved Hide resolved
@@ -834,6 +834,11 @@ namespace vcpkg
.map([](Path&& dot_git_parent) { return std::move(dot_git_parent) / ".git"; });
}

bool VcpkgPaths::is_overlay_port(const std::string& port_name) const
{
return Util::contains(overlay_ports, port_name);
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure overlay_ports is a list of directories and thus this change won't behave correctly. Can you add tests for this?

Copy link
Contributor Author

@Thomas1664 Thomas1664 May 15, 2024

Choose a reason for hiding this comment

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

I'm pretty sure overlay_ports is a list of directories and thus this change won't behave correctly.

At least the name and type suggests that these are in fact port names.

Can you add tests for this?

Do you mean unit tests or e2e tests?

Copy link
Member

Choose a reason for hiding this comment

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

At least the name and type suggests that these are in fact port names.

I believe the name is based on the name of the switch being --overlay-ports.

Do you mean unit tests or e2e tests?

Of course unit tests are better but I doubt we have great ways to do that.

@@ -834,6 +834,13 @@ namespace vcpkg
.map([](Path&& dot_git_parent) { return std::move(dot_git_parent) / ".git"; });
}

bool VcpkgPaths::is_overlay_port(StringView port_name) const
{
return std::find_if(overlay_ports.begin(), overlay_ports.end(), [&port_name](StringView port_dir) {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right either because direct overlay-ports don't care about the directory name. Can you just revert the behavior for this one to how it was before?

@BillyONeal BillyONeal marked this pull request as draft May 29, 2024 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants