Don't fail on non-existant already installed ports. #516

Neumann-A · 2022-04-22T07:59:16Z

The error msg is also never helpful.

fixes microsoft/vcpkg#24302
fixes microsoft/vcpkg#15836 (again)

Error msg was also wrong.

src/vcpkg/registries.cpp

BillyONeal · 2022-04-27T02:30:17Z

Do you have ideas on writing an end to end test for this to ensure we don't break it again in the future?

dg0yt · 2022-04-27T05:11:38Z

Don't fail on non-existant already installed ports.

Yesterday vcpkg failed for me on non-existent already installed port feature...

Neumann-A · 2022-04-27T07:04:34Z

Do you have ideas on writing an end to end test for this to ensure we don't break it again in the future?

@BillyONeal
I locally did:

vcpkg install zlib
**move ports/zlib to a location vcpkg does not search**
vcpkg install szip # -> fails without this PR

but you could just have empty ports A and B without any dependencies.
Install A -> remove the folder for A -> Try to install B.

Yesterday vcpkg failed for me on non-existent already installed port feature...

@dg0yt:
doubt this is covered by this PR.
Test would be. Add a Feature to A's manifest
Install A[feature] -> remove the feature from A's manifest -> Try to install B.

dg0yt · 2022-04-27T07:35:52Z

For testing non-existant ports: They also come when first installing from overlay-ports, then installing a regular port.
(Try that with cmake-user from scripts/test-ports.)

doubt this [non-existant features] is covered by this PR.

Just reported this for completeness.

azure-pipelines/end-to-end-tests-dir/build-test-ports.ps1

…installed_ports

Neumann-A · 2022-05-11T14:14:32Z

@BillyONeal: What is the internal status of this PR?

BillyONeal · 2022-05-19T02:03:58Z

@BillyONeal: What is the internal status of this PR?

No specific / different "internal" state. (The only internal things going on here are those which touch code signing for hopefully apparent reasons :) )

I didn't merge this before because I didn't have a good understanding of the impact: this effectively completely removes error handling out from underneath all dependency resolution etc. which may be important. It is likely that the correct fix is to prevent getting to this place in the first place rather than yanking the error handling.

An e2e test would be helpful if you'd be willing to add one although I don't think that's the limiting "reactant" right now.

Neumann-A · 2022-05-19T06:37:12Z

this effectively completely removes error handling out from underneath all dependency resolution etc. which may be important.

That would be a bug in vcpkg using this function. Since the return value is Optional<Version> the caller has the responsiblity to check and throw if an version is required.

An e2e test would be helpful if you'd be willing to add one although I don't think that's the limiting "reactant" right now.

If you tell me which case you are afraid of. TestRegistryImplementation for example always returns a nullopt for a call to get_baseline_version.

BillyONeal · 2022-05-19T23:38:26Z

That would be a bug in vcpkg using this function. Since the return value is Optional<Version> the caller has the responsiblity to check and throw if an version is required.

Right, but that makes the impact of this review on a ton of code that is not in the review, which is difficult to analyze without a good understanding of the surrounding code, which I do not have.

If you tell me which case you are afraid of. TestRegistryImplementation for example always returns a nullopt for a call to get_baseline_version.

I mean for the bug you're actually fixing.

Neumann-A · 2022-05-19T23:44:46Z

I mean for the bug you're actually fixing.

I don't understand. The test is already there?:

Run-Vcpkg --overlay-ports="$PSScriptRoot/../e2e_ports/overlays" install vcpkg-empty-port
Run-Vcpkg --overlay-ports="$PSScriptRoot/../e2e_ports" install vcpkg-internal-e2e-test-port  <--- here vcpkg does not see the manifest for vcpkg-empty-port any more; Would throw an error with current vcpkg.
if ($IsWindows) {
    Run-Vcpkg --overlay-ports="$PSScriptRoot/../e2e_ports" install vcpkg-find-acquire-program
}
Throw-IfFailed

BillyONeal · 2022-05-19T23:46:07Z

I don't understand. The test is already there?:

Ah I thought that was just fixing my stupidity since we talked about that in this thread 😅

ras0219-msft · 2022-05-20T15:37:34Z

Ok, with this change (44b6add) on Linux I get:

$ vcpkg x-set-installed zlib
# successful install

$ mv ports/zlib ports/zlib2

$ vcpkg install zlib
Computing installation plan...
Error: while loading zlib:
The port directory (/workspaces/vcpkg-tool/vcpkg/ports/zlib) does not exist
Error: while loading control file for zlib:x64-linux: Error: unable to get baseline for port zlib.
Please run "./vcpkg remove zlib:x64-linux" and re-attempt.
note: updating vcpkg by rerunning bootstrap-vcpkg may resolve this failure.

$ vcpkg install curl # depends on zlib
Computing installation plan...
Error: while loading zlib:
The port directory (/workspaces/vcpkg-tool/vcpkg/ports/zlib) does not exist
Error: while loading control file for zlib:x64-linux: Error: unable to get baseline for port zlib.
Please run "./vcpkg remove zlib:x64-linux" and re-attempt.
note: updating vcpkg by rerunning bootstrap-vcpkg may resolve this failure.

$ vcpkg install rapidjson # doesn't depend on zlib
Computing installation plan...
Error: while loading zlib:
The port directory (/workspaces/vcpkg-tool/vcpkg/ports/zlib) does not exist
The following packages will be built and installed:
    rapidjson[core]:x64-linux -> 2020-09-14#2
  * vcpkg-cmake[core]:x64-linux -> 2022-01-19
  * vcpkg-cmake-config[core]:x64-linux -> 2022-02-06
# successful build and install

$ vcpkg install rapidjson # doesn't depend on zlib
Computing installation plan...
Error: while loading zlib:
The port directory (/workspaces/vcpkg-tool/vcpkg/ports/zlib) does not exist
The following packages are already installed:
    rapidjson[core]:x64-linux -> 2020-09-14#2
rapidjson:x64-linux is already installed

A design question we need to answer is where (if anywhere) should vcpkg behave differently between malformed ports versus missing ports? Today, overlays have this difference (missing port means go to next overlay, malformed port means halt with error). Overlays don't call this API so it doesn't directly impact the needed contract, but I'm not confident that this is always the case.

Assuming that baselines should never act differently in this case, I think the right fix is that get_baseline() really needs to return an Expected<> instead of an Optional<>. That way any callers can offer up the detailed error message and we don't need to print it here.

Neumann-A · 2022-05-20T15:56:24Z

I think from the errors it is clear that you still get:
Error: while loading control file for zlib:x64-linux: Error: unable to get baseline for port zlib.
if zlib is needed If the user wants more info just let --debug do the trick?
Should I thus apply: #516 (comment) ? So you won't see an error if zlib is not required?

ras0219-msft

I think it's very important that the specific error information is properly propagated. It's pretty user hostile to say "unable to get baseline for port zlib; pass --debug to get the actual error message" when we have it right there and there's no compelling reason to not display it.

I've pushed a change that does this:

$ vcpkg install curl
Computing installation plan...
Error: while loading control file for zlib:x64-linux:
Error: while loading /workspaces/vcpkg-tool/vcpkg/ports/zlib/vcpkg.json:
:6:38: error: Trailing comma in an object
    on expression:   "homepage": "https://www.zlib.net/",
                                                        ^


Please run "./vcpkg remove zlib:x64-linux" and re-attempt.
note: updating vcpkg by rerunning bootstrap-vcpkg may resolve this failure.

And it removes the non-essential error messages in unrelated paths:

$ vcpkg install rapidjson
Computing installation plan...
The following packages are already installed:
    rapidjson[core]:x64-linux -> 2020-09-14#2
rapidjson:x64-linux is already installed

Plus a test to ensure that there are no error messages displayed on the unrelated path. LGTM with this additional testing and change.

src/vcpkg/sourceparagraph.cpp

…errors

BillyONeal

LGTM provided that the loop actually is tested to work

ras0219-msft · 2022-05-23T20:06:35Z

Thanks everyone! I actually made sure to exercise the code and it works now :)

Don't fail on nonexistant installed ports.

8b5b8d8

Error msg was also wrong.

Neumann-A commented Apr 22, 2022

View reviewed changes

src/vcpkg/registries.cpp Outdated Show resolved Hide resolved

Neumann-A changed the title ~~Don't fail on non-existant installed ports.~~ Don't fail on non-existant already installed ports. Apr 22, 2022

Neumann-A mentioned this pull request Apr 22, 2022

Let vcpkg install check only the relevant ports instead of all microsoft/vcpkg#24302

Closed

Neumann-A commented Apr 22, 2022

View reviewed changes

src/vcpkg/registries.cpp Outdated Show resolved Hide resolved

add e2e test

c4acf8c

Neumann-A mentioned this pull request Apr 27, 2022

Test e2e test noexistent ports #527

Closed

Neumann-A commented Apr 27, 2022

View reviewed changes

azure-pipelines/end-to-end-tests-dir/build-test-ports.ps1 Outdated Show resolved Hide resolved

add Throw-IfFailed

fc096b9

Neumann-A commented Apr 27, 2022

View reviewed changes

azure-pipelines/end-to-end-tests-dir/build-test-ports.ps1 Outdated Show resolved Hide resolved

Alexander Neumann added 2 commits May 4, 2022 15:26

Merge remote-tracking branch 'upstream/main' into ignore_nonexistent_…

e90ce35

…installed_ports

add 'if ($IsWindows)' and install another port for testing

44b6add

Retain detailed error information when finding the baseline fails

3ff332c

ras0219-msft approved these changes May 20, 2022

View reviewed changes

ras0219-msft mentioned this pull request May 20, 2022

Localize ParseControlErrorInfo. #521

Merged

apply diff from ci

20b647e

BillyONeal reviewed May 20, 2022

View reviewed changes

src/vcpkg/sourceparagraph.cpp Show resolved Hide resolved

src/vcpkg/sourceparagraph.cpp Outdated Show resolved Hide resolved

Avoid allocations in print_error_message

b76271a

Neumann-A commented May 23, 2022

View reviewed changes

src/vcpkg/sourceparagraph.cpp Outdated Show resolved Hide resolved

Neumann-A commented May 23, 2022

View reviewed changes

src/vcpkg/sourceparagraph.cpp Show resolved Hide resolved

Fix print_error_message last. Include origin information in manifest …

6e1fed0

…errors

ras0219-msft force-pushed the ignore_nonexistent_installed_ports branch from 993d67b to 6e1fed0 Compare May 23, 2022 17:03

BillyONeal approved these changes May 23, 2022

View reviewed changes

ras0219-msft merged commit 6cdf115 into microsoft:main May 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't fail on non-existant already installed ports. #516

Don't fail on non-existant already installed ports. #516

Neumann-A commented Apr 22, 2022 •

edited

Loading

BillyONeal commented Apr 27, 2022

dg0yt commented Apr 27, 2022

Neumann-A commented Apr 27, 2022 •

edited

Loading

dg0yt commented Apr 27, 2022

Neumann-A commented May 11, 2022

BillyONeal commented May 19, 2022

Neumann-A commented May 19, 2022

BillyONeal commented May 19, 2022

Neumann-A commented May 19, 2022

BillyONeal commented May 19, 2022

ras0219-msft commented May 20, 2022

Neumann-A commented May 20, 2022

ras0219-msft left a comment •

edited

Loading

BillyONeal left a comment

ras0219-msft commented May 23, 2022

Don't fail on non-existant already installed ports. #516

Don't fail on non-existant already installed ports. #516

Conversation

Neumann-A commented Apr 22, 2022 • edited Loading

BillyONeal commented Apr 27, 2022

dg0yt commented Apr 27, 2022

Neumann-A commented Apr 27, 2022 • edited Loading

dg0yt commented Apr 27, 2022

Neumann-A commented May 11, 2022

BillyONeal commented May 19, 2022

Neumann-A commented May 19, 2022

BillyONeal commented May 19, 2022

Neumann-A commented May 19, 2022

BillyONeal commented May 19, 2022

ras0219-msft commented May 20, 2022

Neumann-A commented May 20, 2022

ras0219-msft left a comment • edited Loading

Choose a reason for hiding this comment

BillyONeal left a comment

Choose a reason for hiding this comment

ras0219-msft commented May 23, 2022

Neumann-A commented Apr 22, 2022 •

edited

Loading

Neumann-A commented Apr 27, 2022 •

edited

Loading

ras0219-msft left a comment •

edited

Loading