feat: add v3-essential to support arch-specific essentials #246
Conversation
v3-essential is a new field present for Chisel formats "v2" and "v1". Note that this does not affect how Chisel does conflict resolution across dependencies, i.e. Chisel will not take into account architecture specifics to be smarter about conflicts. IMPORTANT: This commit changes how yaml is marshalled for tools such as chisel info. The values of `essential` and `v3-essential` are both marshaled into `v3-essential` to make it clear that is the expected archive representation.
|
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
This is very good, Alberto, thank you.
Only some minors about the error message.
Also, a comment on this:
IMPORTANT: This commit changes how yaml is marshalled for tools such as chisel info. The values of essential and v3-essential are both marshaled into v3-essential to make it clear that is the expected archive representation.
This is actually a breaking change. It may well be that the likelyhood of breakage isn't worth the cost and complexity of doing anything else. But we should at least see it as it is: we're changing the format in an unsupported way, in a very particular place.
internal/setup/yaml.go
Outdated
| if _, ok := yamlPkg.V3Essential[refName]; ok { | ||
| // This check is only needed because the list format can contain | ||
| // duplicates. It should be removed when format "v2" is deprecated. | ||
| return nil, fmt.Errorf("package %s defined with redundant essential slice: %s", pkgName, refName) |
There was a problem hiding this comment.
s/%s/%q/, as the package name is naked and the word can mix confusingly in the message.
The message also feels unnecessarily vague. It could be straight on:
package %q repeats %s in both "essential" (compatibility) and "v3-essential" (arch support)
There was a problem hiding this comment.
There are actually more cases where this error message is returned. Because essential is a list at the moment, it means that it can have duplicates on its own:
summary: "Duplicated slice essentials",
input: map[string]string{
"slices/mydir/mypkg.yaml": `
package: mypkg
slices:
slice1:
essential:
- mypkg_slice2
- mypkg_slice2
slice2:
`,
},
There was a problem hiding this comment.
I agree that the error message could be simpler so I changed it per your other suggestion to: slice mypkg_slice1 repeats mypkg_slice2 in essential fields for the test case above. We could be more specific, but I think the message is descriptive enough to cover all errors due to duplication in a unified way. Let me know what you think.
2 participants
v3-essential is a new field present for Chisel formats "v2" and "v1". Note that this does not affect how Chisel does conflict resolution across dependencies, i.e. Chisel will not take into account architecture specifics to be smarter about conflicts.
BREAKING CHANGE: This commit changes how yaml is marshalled for tools such as chisel info. The values of
essentialandv3-essentialare both marshaled intov3-essential.