-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
envconfig: fix unhandled exception when cross-file lacks required keys #14397
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
Conversation
Thanks for the nice commit message! The change is fine but if you want to add a unit test, it would be nice to have. |
Can you update the commit message to mention that this regressed in commit b0d2a92 The commit message doesn't really explain why this is a fix (the context from that commit will help, and the comment I left on the linked ticket). This will make it hard to tell from the commit log, why the change was made. |
@eli-schwartz Thank you for your feedback. I've amended the commit message to include the information about regression in commit b0d2a92 and provided more context on why this fix works. I've force-pushed the amended commit with the updated message. Could you please review and confirm if the commit message is now acceptable? Once we agree on the commit message, I plan to add a separate commit with unit tests as suggested by @bonzini. |
Nit:
This isn't really true. The minimum (required) literal doesn't contain non required optional keys, that's why it can still be used in the fix. But the previous check was that the provided keys were a strict subset of the minimum required keys. The strict subset logic broke down once the provided keys aren't a strict subset anymore, but an overlapping set with disjoint elements on both sides of the comparison. Rewriting the logic to use "present in required but not present in provided" fixed that, and in effect removed the accidental implicit check "and not any {{ present in provided but not present in required }}" |
Fix the unhandled KeyError exception that occurs when cross-compilation configuration files are missing required parameters (such as 'endian'). This issue was introduced in commit b0d2a92 (PR mesonbuild#11692), where the key validation logic didn't properly handle the relationship between provided and required keys: - Previously, the code used `set(literal) < minimum_literal` to check if provided keys were a strict subset of the required keys in minimum_literal - This validation logic broke down when the provided keys weren't a strict subset anymore, but rather an overlapping set with disjoint elements on both sides - When required keys were missing, the code continued execution and later threw an unhandled KeyError when trying to access the non-existent keys Changed the condition check from: if set(literal) < minimum_literal: to: if minimum_literal - set(literal): This new check specifically identifies keys that are "present in required but not present in provided", providing users with clear error messages instead of raising unhandled exceptions. This change also removes the implicit requirement that "provided keys must not contain any keys not present in the required set" - allowing for optional keys to exist in the provided configuration. Fixes mesonbuild#14385
@eli-schwartz Thank you for the feedback. I've updated the commit message to more accurately reflect the technical nature of the issue: The revised message now properly explains that:
This better reflects the actual fix implemented, which addresses the logical issue where validation failed when dealing with overlapping key sets instead of strict subsets. |
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.
Thanks, lgtm.
Problem
When users forget to specify the required
endian
parameter in a cross-compilation configuration file, Meson currently throws an unhandledKeyError
exception. This creates confusion as the error message doesn't clearly indicate what's missing, and instead suggests that this is a Meson bug that should be reported.Solution
Modified the condition check in the
MachineInfo.from_literal
method from:to:
This ensures that when mandatory fields are missing, users receive a clear error message explicitly stating which fields are missing, rather than encountering an unhandled exception.
Related Issue
Fixes #14385