fix: re-compute buildhash if config/kind is changed in on_load#7315
fix: re-compute buildhash if config/kind is changed in on_load#7315Willaaaaaaa wants to merge 4 commits intoxmake-io:devfrom
on_load#7315Conversation
Summary of ChangesHello @Willaaaaaaa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the build hash for packages was not consistently updated when package configurations, such as the package type or specific settings, were dynamically altered within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the issue of re-computing the build hash when a package's kind or configs are modified in on_load. The introduction of the _CONFIG_DIRTY flag and its handling in _instance:_load is a solid approach. The fix to correctly handle false configuration values by replacing or with an explicit if value == nil check is also a great improvement. I also appreciate the various typo fixes throughout the codebase.
I have one suggestion to make the logic for handling kind changes more robust by considering the case where the kind is changed from binary to another type.
| if name == "kind" then | ||
| -- the package kind is modified, the buildhash need to be re-computed | ||
| self._CONFIG_DIRTY = true | ||
| if (...) == "binary" then | ||
| -- remove shared config if this package is originally a library | ||
| local configs = self:configs() | ||
| if configs then | ||
| -- this can remove shared config from `manifest.txt` | ||
| configs.shared = nil | ||
| end | ||
| local requireinfo = self:requireinfo() | ||
| if requireinfo then | ||
| requireinfo.ignored_configs_for_buildhash = requireinfo.ignored_configs_for_buildhash or {} | ||
| table.insert(requireinfo.ignored_configs_for_buildhash, "shared") | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The current logic correctly handles changing a package's kind to "binary" by adding "shared" to ignored_configs_for_buildhash. However, it doesn't handle the case where the kind is changed from "binary" to another type (e.g., "library"). In that scenario, "shared" would remain in ignored_configs_for_buildhash, which could lead to incorrect build hashes if the shared configuration is later modified for the library.
To make this more robust, you should also handle the else case to remove "shared" from ignored_configs_for_buildhash when the kind is not "binary".
if name == "kind" then
-- the package kind is modified, the buildhash need to be re-computed
self._CONFIG_DIRTY = true
if (...) == "binary" then
-- remove shared config if this package is originally a library
local configs = self:configs()
if configs then
-- this can remove shared config from `manifest.txt`
configs.shared = nil
end
local requireinfo = self:requireinfo()
if requireinfo then
requireinfo.ignored_configs_for_buildhash = requireinfo.ignored_configs_for_buildhash or {}
table.insert(requireinfo.ignored_configs_for_buildhash, "shared")
end
else
local requireinfo = self:requireinfo()
if requireinfo and requireinfo.ignored_configs_for_buildhash then
table.remove_if(requireinfo.ignored_configs_for_buildhash, function(_, v) return v == "shared" end)
end
end
end
|
buildhash 暂时不要去动，我知道有这个问题，但这个不是这么好改的。 on_load 执行之前，package 内部 _load_package 很多地方都已经用上了 buildhash ，比如 installdir ，而且还已经创建了目录，而且还有 package cache 存在，都会被影响。 即使你后面刷新了 buildhash ，也会导致很多地方状态不一致，引出各种问题。另外现在的改法，很多都是对 kind 硬处理，更不好维护。 |
|
目前 |
Don't touch buildhash for now. I know there is this problem, but it is not easy to change. Before on_load is executed, buildhash has been used in many places within the package _load_package, such as installdir, and the directory has been created, and the package cache exists, so it will be affected. Even if you refresh the buildhash later, the status will be inconsistent in many places and lead to various problems. In addition, many of the current reform methods require hard processing of kind, which is even more difficult to maintain. |
at present |
3 participants
Fix #7314