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

BillyONeal · 2023-10-28T02:22:41Z

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

ras0219-msft

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

src/vcpkg-test/manifests.cpp

ras0219-msft · 2023-10-30T20:49:52Z

src/vcpkg-test/versions.cpp

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


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.

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'

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

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

ras0219-msft · 2023-10-30T20:55:21Z

src/vcpkg/binarycaching.cpp

@@ -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;


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

src/vcpkg/commands.build.cpp

…dencyConstraint{}

BillyONeal · 2023-10-30T23:08:37Z

Please look at #1259 first

ras0219-msft

I think this all LGTM once #1259 is resolved.

ras0219-msft · 2023-10-31T22:37:20Z

src/vcpkg/commands.build.cpp

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


Suggested change

auto version =

build_info.version =

ras0219-msft · 2023-10-31T22:43:02Z

include/vcpkg/base/message-data.inc.h

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


I feel like either:

This comment should be very common and we should have a placeholder for "user input"

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

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.

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

ras0219-msft · 2023-10-31T22:47:42Z

src/vcpkg/binarycaching.cpp

-        , 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())


Suggested change

action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO).source_control_file->to_version())

action.version())

ras0219-msft · 2023-10-31T22:50:23Z

src/vcpkg/binarycaching.cpp

                           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,


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(),

ras0219-msft · 2023-10-31T22:53:42Z

src/vcpkg/commands.update.cpp

-                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;


Suggested change

const auto& latest_version = p_scfl->source_control_file->core_paragraph->version;

const auto& latest_version = p_scfl->to_version();

ras0219-msft · 2023-10-31T22:55:17Z

src/vcpkg/commands.upgrade.cpp

                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)


Suggested change

if (control_paragraph.version == installed_paragraph.version)

if (control_file.to_version() == installed_paragraph.version)

ras0219-msft · 2023-10-31T23:39:26Z

src/vcpkg/sourceparagraph.cpp

+                }
+                else
+                {
+                    r.add_generic_error(type_name(), msg::format(msgVersionConstraintPortVersionMustBePositiveInteger));


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

BillyONeal · 2023-11-01T03:39:37Z

Now depends on #1261

BillyONeal added 4 commits October 27, 2023 17:08

Make version fields editable.

c3e9b49

Use Version in BinaryParagraph.

214d170

Use Version in DependencyConstraint and SourceParagraph.

f7aceb1

Fix test failures.

963edaa

ras0219-msft reviewed Oct 30, 2023

View reviewed changes

BillyONeal added 2 commits October 30, 2023 15:49

DependencyConstraint{VersionConstraintKind::None, Version{}} => Depen…

1d70ca3

…dencyConstraint{}

Only use version text in UrlTemplate.

de7aae8

BillyONeal added the depends:different-pr This PR depends on a different PR which has been filed label Oct 30, 2023

ras0219-msft approved these changes Oct 31, 2023

View reviewed changes

BillyONeal removed the depends:different-pr This PR depends on a different PR which has been filed label Nov 3, 2023

BillyONeal added 3 commits November 3, 2023 16:23

Merge remote-tracking branch 'origin/main' into more-versions

f606ddb

CR feedback

f646691

Merge remote-tracking branch 'origin/main' into more-versions

d78d5b4

BillyONeal merged commit 0e69395 into microsoft:main Nov 7, 2023
5 checks passed

BillyONeal deleted the more-versions branch November 7, 2023 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

BillyONeal commented Oct 28, 2023 •

edited

ras0219-msft left a comment

ras0219-msft Oct 30, 2023

BillyONeal Oct 30, 2023

ras0219-msft Oct 31, 2023 •

edited

BillyONeal Nov 1, 2023 •

edited

ras0219-msft Oct 30, 2023

BillyONeal commented Oct 30, 2023

ras0219-msft left a comment

ras0219-msft Oct 31, 2023

ras0219-msft Oct 31, 2023

BillyONeal Nov 4, 2023

ras0219-msft Oct 31, 2023

ras0219-msft Oct 31, 2023

ras0219-msft Oct 31, 2023

ras0219-msft Oct 31, 2023

ras0219-msft Oct 31, 2023

BillyONeal commented Nov 1, 2023

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

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

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

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

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

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

Conversation

BillyONeal commented Oct 28, 2023 • edited

ras0219-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ras0219-msft Oct 31, 2023 • edited

Choose a reason for hiding this comment

BillyONeal Nov 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented Oct 30, 2023

ras0219-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented Nov 1, 2023

BillyONeal commented Oct 28, 2023 •

edited

ras0219-msft Oct 31, 2023 •

edited

BillyONeal Nov 1, 2023 •

edited