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

Use Version instead of {raw_version,port_version} in more places. #1253

Merged
merged 9 commits into from
Nov 7, 2023

Conversation

BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Oct 28, 2023

This isn't super high priority, but I noticed it based on one of @vicroms ' code review comments.

I also think we should think hard about eliminating SchemedVersion given that people don't seem to like that being consistently used...

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Reviewed through commands.build.cpp -- will pick up later.

src/vcpkg-test/manifests.cpp Outdated Show resolved Hide resolved

TEST_CASE ("parse version", "[versions]")
{
CHECK(Version::parse("").value_or_exit(VCPKG_LINE_INFO) == Version{});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CHECK(Version::parse("").value_or_exit(VCPKG_LINE_INFO) == Version{});
CHECK(Version::parse("") == Version{});

Can we make this work somehow? Calling .value_or_exit() is a really nasty smell in the test code because it makes "assertion failure" a huge mess.

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 disagree; value or exit assertion failures are easy, since they print the exact line that's a problem. The only thing you get out of trying to eliminate them is when you have several test failures that you could get all at once, but our unit tests are already fast enough that it does not make sense to worry about anything beyond 'everything passed' or 'everything's doomed'

Copy link
Contributor

@ras0219-msft ras0219-msft Oct 31, 2023

Choose a reason for hiding this comment

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

but our unit tests are already fast enough that it does not make sense to worry about anything beyond 'everything passed' or 'everything's doomed'

It's more complicated than that and not just about speed. If you can see all the unit test failures, you can find the "simplest" unit test that fails and work on that one first. This gives you the most information to identify the fault. However, if the first unit test that runs causes the entire harness to abort, you lose a lot of "diagnostic power".

I have personally found tests that abort the entire process to be very frustrating for this reason and took steps to first make them not abort.

since they print the exact line that's a problem.

They print the exact line of the assertion that fails, but certainly not the line inside the code where the problem originates. If you just want to break on the first assertion, that's easily done via vcpkg-test -b1 which also supports CHECK()s that aren't value_or_exit().

Copy link
Member Author

@BillyONeal BillyONeal Nov 1, 2023

Choose a reason for hiding this comment

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

have personally found tests that abort the entire process to be very frustrating for this reason and took steps to first make them not abort.

The status quo is that lots of tests see ExpectedL and that means lots of tests can't meaningfully continue. And lots of tests call value_or_exit.

There were tests that once upon a time called CHECK(foo.has_value()) but then couldn't do anything meaningful, so such a check was pointless (as the line after the check would crash)

They print the exact line of the assertion that fails, but certainly not the line inside the code where the problem originates.

The assertion that fails is better than what other parts of the test harness do to avoid value_or_exit. Hoisting this test into another function that does the value_or_default-ness is common and destroys context as to which test failed.

that's easily done via vcpkg-test -b1 which also supports CHECK()s that aren't value_or_exit().

That's not what PR build does so I view it as of limited value. If this is a local run, I can easily run under a debugger and stop on whatever I want. The time when the console output matters is when you can't debug it because it happens on a platform annoying to test.

I'm not opposed in general to some solution that avoids value_or_exit here but I am opposed to any that lose context as to which line fails, and as far as I know we don't have tech in Catch2 to fix that.

src/vcpkg/binarycaching.cpp Outdated Show resolved Hide resolved
@@ -2416,7 +2416,7 @@ std::string vcpkg::generate_nuspec(const Path& package_dir,
{
auto& spec = action.spec;
auto& scf = *action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO).source_control_file;
auto& version = scf.core_paragraph->raw_version;
auto& version = scf.core_paragraph->version;
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that this is a benign behavior change -- the NuGet description will now (correctly) track the port-version.

src/vcpkg/commands.build.cpp Outdated Show resolved Hide resolved
@BillyONeal BillyONeal added the depends:different-pr This PR depends on a different PR which has been filed label Oct 30, 2023
@BillyONeal
Copy link
Member Author

Please look at #1259 first

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

I think this all LGTM once #1259 is resolved.

auto port_version_text = parser.optional_field(Fields::PORT_VERSION);
if (!version_text.empty())
{
auto version =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auto version =
build_info.version =

DECLARE_MESSAGE(PortVersionNonnegativeInteger,
(msg::value),
"{value} is what the user entered",
"port-version (after the '#') must be a non-negative integer, but was {value}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like either:

  1. This comment should be very common and we should have a placeholder for "user input"
  2. We should avoid the construction of reflecting the user's text back to them within the error message and it should instead be ancillary information on following rows (like how parse errors report the user's input on extra rows).

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I argue the placeholder for "user input" is {value}. That's the placeholder for what happens when we can't say anything about what it looks like because we have failed to make sense of it. I looked into trying to create a separate message arg for it, but this turns out to require a ton of surgery to the entire message system in order to tolerate a description which is not an example to be associated with a message arg, which I don't think is warranted here. I'm not opposed in general to a solution which might avoid repeating this sometimes but I don't think it's worth the cost to do such surgery at this time.
  2. In my experience without reflecting what the user said back at them in this context, the user often has very little idea what happened. I could see a future where instead of reflecting in the message we show contextual information or file/line number, but that is not what the emitting place of this code can currently see. I also think implicating something like that over this "hey these two fields should use the type we use for them" change would be a bit overkill...

, raw_version(action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO)
.source_control_file->core_paragraph->raw_version)
, version(
action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO).source_control_file->to_version())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO).source_control_file->to_version())
action.version())

Comment on lines 2574 to 2575
action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO)
.source_control_file->core_paragraph->raw_version,
.source_control_file->core_paragraph->version,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO)
.source_control_file->core_paragraph->raw_version,
.source_control_file->core_paragraph->version,
action.version(),

auto latest_version = Version(latest_pgh.raw_version, latest_pgh.port_version);
auto installed_version = Version(pgh->package.version, pgh->package.port_version);
if (latest_version != installed_version)
const auto& latest_version = p_scfl->source_control_file->core_paragraph->version;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const auto& latest_version = p_scfl->source_control_file->core_paragraph->version;
const auto& latest_version = p_scfl->to_version();

const auto& installed_paragraph = (*installed_status)->package;
auto installed_version = Version(installed_paragraph.version, installed_paragraph.port_version);
if (control_version == installed_version)
if (control_paragraph.version == installed_paragraph.version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (control_paragraph.version == installed_paragraph.version)
if (control_file.to_version() == installed_paragraph.version)

}
else
{
r.add_generic_error(type_name(), msg::format(msgVersionConstraintPortVersionMustBePositiveInteger));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only reason that Version::parse() could fail? Choosing the specific error message here seems a bit far from the source.

@BillyONeal
Copy link
Member Author

Now depends on #1261

@BillyONeal BillyONeal removed the depends:different-pr This PR depends on a different PR which has been filed label Nov 3, 2023
@BillyONeal BillyONeal merged commit 0e69395 into microsoft:main Nov 7, 2023
5 checks passed
@BillyONeal BillyONeal deleted the more-versions branch November 7, 2023 19:34
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