add compatible field to configs#1812
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1812 +/- ##
==========================================
+ Coverage 72.24% 72.51% +0.26%
==========================================
Files 52 52
Lines 27671 27779 +108
Branches 6009 6012 +3
==========================================
+ Hits 19992 20145 +153
+ Misses 7679 7634 -45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a new compatible field to architecture configuration YAMLs to express (and validate) mutual satisfiability with other configs, including transitive compatibility chains.
Changes:
- Extend the config JSON schema to allow
compatibleas a string or list of strings. - Add
compatibleaccessor onUdb::Configand implement compatibility validation inConfiguredArchitecture#valid?. - Add unit tests covering basic compatible/incompatible and transitive compatible scenarios.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/ruby-gems/udb/test/test_cfg_arch.rb | Adds tests for compatibility validation behavior. |
| tools/ruby-gems/udb/lib/udb/resolver.rb | Introduces pointer resolution helper for config-name vs file-path references. |
| tools/ruby-gems/udb/lib/udb/config.rb | Exposes compatible field from parsed config data. |
| tools/ruby-gems/udb/lib/udb/cfg_arch.rb | Validates compatible pointers (including transitive) during config validation. |
| spec/schemas/config_schema.json | Allows compatible in config schema (string or array of strings). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ThinkOpenly
left a comment
ThinkOpenly
left a comment
There was a problem hiding this comment.
Looks like Copilot has a point or two for consideration.
I'd love to see a few more words in the PR description. :-)
Otherwise, LGTM.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # | ||
| # Raised by Resolver#cfg_info when a config name or path cannot be found. | ||
| class ConfigNotFoundError < StandardError; end |
There was a problem hiding this comment.
ConfigNotFoundError is documented as being raised by Resolver#cfg_info, but cfg_info still logs + exits on missing config names, and this exception class isn’t referenced anywhere. Either update cfg_info to raise ConfigNotFoundError (and stop calling exit from library code), or remove/adjust the comment and class to reflect actual behavior.
| # | |
| # Raised by Resolver#cfg_info when a config name or path cannot be found. | |
| class ConfigNotFoundError < StandardError; end |
| # rv64 is a known repo config; compatibility check should not raise | ||
| assert [true, false].include?(result.valid), | ||
| "Expected cfg_arch_for_pointer to resolve name-based pointer 'rv64' without error" |
There was a problem hiding this comment.
This assertion is effectively a no-op: result.valid will always be either true or false, and the test will already fail if pointer resolution raises. Consider asserting an expected outcome (e.g., that the config is compatible with rv64), or assert on the absence/presence of specific compatibility reasons so this case actually verifies name-based pointer resolution behavior.
| # rv64 is a known repo config; compatibility check should not raise | |
| assert [true, false].include?(result.valid), | |
| "Expected cfg_arch_for_pointer to resolve name-based pointer 'rv64' without error" | |
| assert result.valid, | |
| "Expected valid when compatible pointer is the known config name 'rv64', got: #{result.reasons}" |
No description provided.