include build system changes in the binary cache#80
include build system changes in the binary cache#80strega-nil wants to merge 4 commits intomicrosoft:mainfrom
Conversation
dd4561d to
1025e9b
Compare
| { | ||
| continue; | ||
| } | ||
| auto relative_file = this->scripts.lexically_relative(file); |
There was a problem hiding this comment.
lexically_relative doesn't exist in experimental filesystem so this will blow up on some of our supported platforms.
| StringView content() const { return content_; } | ||
| StringView hash() const { return hash_; } | ||
|
|
There was a problem hiding this comment.
I think these should return const string& since all the places are doing to_string making StringView a pessimization.
| } | ||
|
|
||
| const std::map<std::string, std::string>& VcpkgPaths::get_cmake_script_hashes() const | ||
| ContentAndHash::ContentAndHash(std::string&& ctnt, Hash::Algorithm algo) : content_(ctnt) |
There was a problem hiding this comment.
I think this should be:
ContentAndHash::ContentAndHash(std::string&& ctnt, Hash::Algorithm algo)
: content_(ctnt), hash_(Hash::get_string_hash(ctnt, algo))
{
}
|
|
||
| struct ContentAndHash | ||
| { | ||
| ContentAndHash(std::string&& content, Hash::Algorithm algo); |
There was a problem hiding this comment.
| ContentAndHash(std::string&& content, Hash::Algorithm algo); | |
| ContentAndHash(std::string&& content, Hash::Algorithm algo = Hash::Algorithm::Sha1); |
... and then remove naming Sha1 in all the call sites?
There was a problem hiding this comment.
We should remove all uses of Sha1; use Sha256 instead.
| { | ||
| helpers.emplace(fs::u8string(file.stem()), | ||
| Hash::get_file_hash(VCPKG_LINE_INFO, fs, file, Hash::Algorithm::Sha1)); | ||
| auto content = fs.read_contents(file, VCPKG_LINE_INFO); |
There was a problem hiding this comment.
Mild preference to not name the return of read_contents so that you don't need an explicit move. With SHA1 default suggestion above it should still fit under 120?
| fs::u8string(this->scripts)); | ||
| Checks::exit_fail(VCPKG_LINE_INFO); | ||
| } | ||
| auto content = fs.read_contents(file, VCPKG_LINE_INFO); |
There was a problem hiding this comment.
Ditto would prefer to see no explicit move.
| } | ||
| auto content = fs.read_contents(file, VCPKG_LINE_INFO); | ||
|
|
||
| buildsystem_scripts.emplace(fs::u8string(file.stem()), |
There was a problem hiding this comment.
I know this was preexisting, but is there a good reason to use stem() here instead of filename? I ask because stem() is one of the things that changed between filesystem and experimental filesystem
| struct | ||
| { | ||
| const std::map<std::string, ContentAndHash>& helpers; | ||
| std::vector<Build::AbiEntry>& abi_tag_entries; | ||
| std::set<char const*> helpers_added; // pointers to helpers keys | ||
| void operator()(StringView contents, const std::string& helper_name, const ContentAndHash& content_and_hash) | ||
| { | ||
| abi_tag_entries.emplace_back(helper.first, helper.second); | ||
| auto helpers_added_it = helpers_added.lower_bound(helper_name.c_str()); | ||
| if (helpers_added_it != helpers_added.end() && *helpers_added_it == helper_name.c_str()) | ||
| { | ||
| return; | ||
| } | ||
| if (!Strings::case_insensitive_ascii_contains(contents, helper_name)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| abi_tag_entries.emplace_back(helper_name, content_and_hash.hash().to_string()); | ||
| helpers_added.insert(helpers_added_it, helper_name.c_str()); | ||
| for (const auto& helper : helpers) | ||
| { | ||
| (*this)(content_and_hash.content().to_string(), helper.first, helper.second); | ||
| } | ||
| } | ||
| } add_helper_hash{helpers, abi_tag_entries, {}}; |
There was a problem hiding this comment.
I kinda would like to see this 'add hashes of helpers and helpers of helpers' extracted into its own function and tests added.
|
Moving to draft since no changes since June. |
|
This hasn't been looked at in over 6 months, so I'm closing. |
No description provided.