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

binarycaching: Add NuGet timeout configuration entry #95

Merged
merged 3 commits into from
Jul 1, 2021
Merged

binarycaching: Add NuGet timeout configuration entry #95

merged 3 commits into from
Jul 1, 2021

Conversation

ekilmer
Copy link
Contributor

@ekilmer ekilmer commented Jun 17, 2021

Related to this issue microsoft/vcpkg#18483

As mentioned in the issue, ideally this timeout value could be configurable, but I'm not sure the best way to approach that in this code base. Help would be greatly appreciated!

Needs the extra second due to some potential issue with how NuGet calculates the timeout:
https://jfrog.com/knowledge-base/how-to-resolve-nuget-push-failures-after-5-minutes-even-though-timeout-value-is-set-as-greater-than-5-minutes-300-seconds/

@ras0219-msft
Copy link
Contributor

Thanks for the PR!

I think we will eventually want this setting to come through the binary caching configuration string. To preserve our ability to fuse multiple NuGet calls together, I think this means it will need to come from a separate entry:

nuget,<url1>;nuget,<url2>;nugettimeout,3601

@ekilmer
Copy link
Contributor Author

ekilmer commented Jun 19, 2021

we will eventually want this setting to come through the binary caching configuration string

Just to confirm: You are referencing the valid source strings section and VCPKG_BINARY_SOURCES environment variable/--binarysource CLI option in the documentation on binary caching, and adding a new keyword nugettimeout?

If so, that makes sense to me! Thank you.

@ekilmer
Copy link
Contributor Author

ekilmer commented Jun 27, 2021

@ras0219-msft I've tried implementing a solution to what you described. I'd appreciate a review when you get a chance. Thank you!

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.

This looks really good to me, thanks for making this PR!

Could you also add the new documentation to docs/users/binarycaching.md?

@ekilmer
Copy link
Contributor Author

ekilmer commented Jun 30, 2021

Could you also add the new documentation to docs/users/binarycaching.md?

I'd be happy to! Just one question, how does that work? It looks like that file lives in the main vcpkg repo, and a PR to update the doc should also bump the vcpkg-tool to a commit that has this PR merged. But a bump to the vcpkg-tool version look like it requires a release (so that Windows users can download a pre-built executable).


It also looks like there is now a conflict with binarycaching.cpp file. I will try to fix that today at some point.

@ekilmer ekilmer changed the title binarycaching: Default NuGet push timeout to one hour binarycaching: Add NuGet timeout configuration entry Jun 30, 2021
* main:
  Clean up vcpkg namespaces (#98)
  vcpkg search: Users should use git pull to get the latest results (#100)
  Respect secrets in debug output for downloads. (#101)
  switch to Casey convention for declarations (#102)
  Fix x-download when downloads isn't in a vcpkg root (#96)
@ras0219-msft ras0219-msft merged commit 78dbb6c into microsoft:main Jul 1, 2021
@ras0219-msft
Copy link
Contributor

Ah, you're absolutely right. Sorry for my confusion! Still getting used to the split repos I suppose :)

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants