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

Fix all download failures claiming that the download was disabled by x-block-origin. #1513

Merged
merged 25 commits into from
Nov 11, 2024

Conversation

JavierMatosD
Copy link
Contributor

The tool currently falsely claims that "Missing [package] and downloads are blocked by x-block-origin"

To repro:

.\vcpkg.exe x-download out.txt --skip-sha512 --url=
https://nonexistent.com/

output:

~/dev/vcpkg-tool % out/build/macos-ci/vcpkg x-download out.txt --skip-sha512 --url=
https://nonexistent.com
error: Missing out.txt and downloads are blocked by x-block-origin.
error: : curl failed to download with exit code 2
curl: (2) no URL specified
curl: try 'curl --help' or 'curl --manual' for more information

Expected:

~/dev/vcpkg-tool % out/build/macos-ci/vcpkg x-download out.txt --skip-sha512 --url=
https://nonexistent.com
error: : curl failed to download with exit code 2
curl: (2) no URL specified
curl: try 'curl --help' or 'curl --manual' for more information

@BillyONeal
Copy link
Member

BillyONeal commented Oct 24, 2024

This output is still broken; we aren't downloading from the destination file name:

example funny output

Can you fix this and show that you've smoke tested at least the following and seen reasonable results? (I understand that writing automated tests for this can be hard because it depends on what is up but if you've got ideas on testing it that would be good...)

excel table of download cases

@JavierMatosD
Copy link
Contributor Author

JavierMatosD commented Oct 24, 2024

Initial smoke tests

File exists

Expect: Message that file exists and matches the SHA

image

Note

BUG: Currently it prints "Download [pkg]". This is bogus

File does not exist

azurl (no), x-block-origin (no), asset-cache (n/a), download (fail)
Expected: Download failure message, nothing about asset caching

image

azurl (no), x-block-origin (no), asset-cache (n/a), download (sha-mismatch)
Expected: Download message with "you might need to configure a proxy" and expected/actual sha
image

Note

Missing the "you might need to configure a proxy"

azurl (no), x-block-origin (no), asset-cache (n/a), download (succeed)
Expected: Download success message, nothing about asset caching

Note

BUG: Currently prints "Downloading [pkg]". Should be download success message.

azurl (no), x-block-origin (yes), asset-cache (n/a), download (n/a)
Expected: Download failure message, nothing about asset caching, x-block-origin complaint

image

azurl (yes), x-block-origin (no), asset-cache (miss), download (fail)
Expected: Download failure message, asset cache named, nothing about x-block-origin

image

Note

The curl messaging is confusing?

azurl (yes), x-block-origin (no), asset-cache (hit), download (n/a)
Expected: Download success message, asset cache named, nothing about x-block-origin

image

azurl (yes), x-block-origin (no), asset-cache (miss), download (sha-mismatch)
Expected: Download message with "you might need to configure a proxy" and expected/actual sha

image

Note

Missing the "you might need to configure a proxy"

azurl (yes), x-block-origin (no), asset-cache (miss), download (succeed)
Expected: Download success message, asset cache upload, nothing about x-block-origin

image

azurl (yes), x-block-origin (yes), asset-cache (miss), download (n/a)
Expected: Download failure message, which asset cache was tried, x-block-origin complaint

image

Note

This prints the x-block-origin complaint two separate times

azurl (yes), x-block-origin (yes), asset-cache (hit), download (n/a)
Expected: Download success message, asset cache named, nothing about x-block-origin

image

}

#azurl (no), x-block-origin (no), asset-cache (n/a), download (sha-mismatch)
#Expected: Download message with "you might need to configure a proxy" and expected/actual sha
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: No "you might need to configure a proxy". I could add back in, but it feels like a potentially misleading message.

Copy link
Member

Choose a reason for hiding this comment

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

Considering it was in the CMake block we are removing I think it should be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BillyONeal BillyONeal closed this Nov 8, 2024
@BillyONeal BillyONeal reopened this Nov 8, 2024
@BillyONeal
Copy link
Member

Closed and reopened to kick CLABot

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.

Only nitpicks here, you can ignore them if you want.

I would really like to see the proxy message return given that we're removing it from CMake land.

}

#azurl (no), x-block-origin (no), asset-cache (n/a), download (sha-mismatch)
#Expected: Download message with "you might need to configure a proxy" and expected/actual sha
Copy link
Member

Choose a reason for hiding this comment

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

Considering it was in the CMake block we are removing I think it should be there.

src/vcpkg/base/downloads.cpp Outdated Show resolved Hide resolved
@BillyONeal BillyONeal changed the title Fix download error bug Fix all download failures claiming that the download was disabled by x-block-origin. Nov 8, 2024
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.

I'm happy with this if the newline replacements that Run-VcpkgAndCaptureOutput already does are removed.

I still believe that it's a bug that we don't emit the proxy message which means CMake is still emitting a separate, different, download failure message, as that might be complaining about proxy when the actual reason is x-block-origin, but this PR is an improvement over the status quo.

azure-pipelines/end-to-end-tests-dir/asset-caching.ps1 Outdated Show resolved Hide resolved
}

#azurl (no), x-block-origin (no), asset-cache (n/a), download (sha-mismatch)
#Expected: Download message with the "you might need to configure a proxy" message and with expected/actual sha
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Downloading example3.html
Failed to download example3.html.
If you are using a proxy, please ensure your proxy settings are correct.
Possible causes are:
1. You are actually using an HTTP proxy, but setting HTTPS_PROXY variable to `https//address:port`.
This is not correct, because `https://` prefix claims the proxy is an HTTPS proxy, while your proxy (v2ray, shadowsocksr, etc...) is an HTTP proxy.
Try setting `http://address:port` to both HTTP_PROXY and HTTPS_PROXY instead.
2. If you are using Windows, vcpkg will automatically use your Windows IE Proxy Settings set by your proxy software. See, https://github.com/microsoft/vcpkg-tool/pull/77
The value set by your proxy might be wrong, or have same `https://` prefix issue.
3. Your proxy's remote server is our of service.
If you've tried directly download the link, and believe this is not a temporay download server failure, please submit an issue at https://github.com/Microsoft/vcpkg/issues
to report this upstream download server failure.
error: https://localhost:1234/foobar.html: curl failed to download with exit code 7
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

curl: (7) Failed to connect to localhost port 1234 after 0 ms: Couldn't connect to server

@BillyONeal BillyONeal merged commit 1393ad8 into microsoft:main Nov 11, 2024
6 checks passed
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.

3 participants