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

General object storage interface for binary caching. #456

Merged
merged 2 commits into from
Mar 30, 2022
Merged

General object storage interface for binary caching. #456

merged 2 commits into from
Mar 30, 2022

Conversation

day253
Copy link
Contributor

@day253 day253 commented Mar 26, 2022

I would like to see the aws and gcs merged such that the tool exe is passed in if the intent is that they truly have the same interface so that we don't forget to update one when changes are made to the other.

This PR is to enable vcpkg users using general object storage interface for binary caching.

@day253
Copy link
Contributor Author

day253 commented Mar 26, 2022

#435
@BillyONeal

@day253 day253 marked this pull request as ready for review March 27, 2022 06:40
@day253
Copy link
Contributor Author

day253 commented Mar 27, 2022

Could you help me about the pipeline? I changed nearly nothing but the pipeline is failed. #457 @BillyONeal

@day253
Copy link
Contributor Author

day253 commented Mar 28, 2022

Could you help me about the pipeline? I changed nearly nothing but the pipeline is failed. #457 @BillyONeal

I this pull request #460 will resolve my problem.

@strega-nil-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vicroms
Copy link
Member

vicroms commented Mar 28, 2022

Hi @day253

For PRs that include localizable strings you don't need to provide translations for each language, we have a team of translators at Microsoft that takes care of that. If you want to submit translation recommendations, please open an issue in the main vcpkg repository.

The process to add localizable strings is to:

  1. declare them with DECLARE_AND_REGISTER_MESSAGE in your code
  2. run the generate-message-map CMake target.

PowerShell Example:

# from the vcpkg-tool directory
mkdir build; cd build
cmake .. -G Ninja
cmake --build . --target generate-message-map

This will generate updated locales/messages.json and locales/messages.en.json that you can commit in your PR.

P.S.: We also have a useful fmt CMake target that will automatically format all code files using vcpkg's format rules.

@day253
Copy link
Contributor Author

day253 commented Mar 29, 2022

Hi @day253

For PRs that include localizable strings you don't need to provide translations for each language, we have a team of translators at Microsoft that takes care of that. If you want to submit translation recommendations, please open an issue in the main vcpkg repository.

The process to add localizable strings is to:

  1. declare them with DECLARE_AND_REGISTER_MESSAGE in your code

  2. run the generate-message-map CMake target.

PowerShell Example:


# from the vcpkg-tool directory

mkdir build; cd build

cmake .. -G Ninja

cmake --build . --target generate-message-map

This will generate updated locales/messages.json and locales/messages.en.json that you can commit in your PR.

P.S.: We also have a useful fmt CMake target that will automatically format all code files using vcpkg's format rules.

Thanks a lot. I have fixed all the review comments. Could please help me review all my changes?

@day253 day253 changed the title merge gcs and aws General onject storage interface for binary caching. Mar 29, 2022
@day253 day253 changed the title General onject storage interface for binary caching. General object storage interface for binary caching. Mar 29, 2022
@day253
Copy link
Contributor Author

day253 commented Mar 29, 2022

cc @coryan for the gcs changes

cc @dave-juicelabs for the aws changes

@day253
Copy link
Contributor Author

day253 commented Mar 29, 2022

cc @ras0219-msft as owner of binary caching

@day253
Copy link
Contributor Author

day253 commented Mar 30, 2022

@ras0219-msft ras0219-msft self-requested a review March 30, 2022 18:29
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 applied Victor's comment and also applied a handful of small tweaks:

  • Use literals directly instead of saving them as std::string members
  • Use StringLiteral/StringView over std::string
  • Factor out Command{paths.get_tool(...)} as a member function.
  • msgObjectStorageToolFailed is used for failures to upload OR download, so I made the message more generic

LGTM, thanks for the PR!

@ras0219-msft ras0219-msft merged commit 934d827 into microsoft:main Mar 30, 2022
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.

5 participants