- Notifications
You must be signed in to change notification settings - Fork177
Feat/support default casdownloader and config path#1790
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Feat/support default casdownloader and config path#1790
Uh oh!
There was an error while loading.Please reload this page.
Conversation
74fec40 to4ae2955CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
4ae2955 to44b9708Compare2d47138 toe5d59f7CompareChange to accept DeviceBuild as argument, which contains more info.
e5d59f7 todf06c11Compare| template<typename T> | ||
| classFlagValue { | ||
| public: | ||
| FlagValue() : default_value_{} {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't provide a default constructor if it's not needed. Without it, if you add a new flag toCasDownloaderFlags but forget to initialize it in the constructor with a default value you'll get a compiler error; otherwise it will just use the default constructor.
| FlagValue() : default_value_{} {} | ||
| explicitFlagValue(const T& default_value) : default_value_(default_value) {} | ||
| // Return by value to avoid dangling references. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It should be ok to returnconst T&. Using that returned reference in a way that causes issues would also cause issues when returning by value. In most cases the returned reference will be use to copy-construct a new variable, but in cases like this the copy-constructor is avoided:
void dosomething(const std::string& str) {...}...dosomething(my_string_flag.value());| // disabled and why, using the same environment-aware formatting that | ||
| // test helpers use. | ||
| LOG(INFO) <<"CAS downloading disabled:" << result.error().FormatForEnv(); | ||
| returnandroid::base::unexpected(std::move(result).error()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can this just bereturn result;?
| // invocation-id flag. Do this only if the invocation-id flag is already | ||
| // present and contains `caller` only. | ||
| for (auto& flag : cas_flags) { | ||
| if (flag.find("-invocation-id=caller=") ==0 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: Useabsl::StartsWith
| boolDefaultConfigPathExists() { | ||
| returnFileExists(std::string(kDefaultCasConfigFilePath)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Delete unused function
| flags1.cache_dir.set_value("/cache/dir"); | ||
| // Verify flags2 is not affected | ||
| if (DefaultDownloaderPathExists()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This test is not hermetic. Consider asserting thatflgas2.downloader_path.value() != flags1.downloader_path.value() instead and deleteDefaultDownloaderPathExists.
Add support for default path for casdownloader and its config file
Change CasDownloader::DownloadFile() to accept DeviceBuild as argument, which contains more info
Support casdownloader flag invocation-id, allows troubleshooting of CAS downloading traffic issues
Log the cause for disabling CAS when CasDownloader::Create fails