Repository navigation
chore: expose manifest reading and jsonwall as packages - #182
Conversation
letFunny
changed the title
cjdcordeiro
left a comment
cjdcordeiro
left a comment
There was a problem hiding this comment.
We'll likely need to add the new licensing headers to each one of these source files
cjdcordeiro
left a comment
cjdcordeiro
left a comment
There was a problem hiding this comment.
thanks for adding the licensing headers. they lgtm
|
Note pls ignore the failed RTD check. I've enabled the automatic generation of PR docs, which will start passing once #185 is merged |
|
I am blocking this until we solve all the questions regarding copyright and licensing. |
| @@ -0,0 +1,89 @@ | |||
| package main | |||
There was a problem hiding this comment.
[Comment for reviewer]: This script for the CI is the only new code, the rest is all moved.
| @@ -0,0 +1,54 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| package testutil | |||
There was a problem hiding this comment.
[Question for reviewer]: I am not fond of naming this one testutil because it could conflict with the regular internal/testutil, however I wanted to convey that this functions are only to be used in tests.
| @@ -0,0 +1,32 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| package util | |||
There was a problem hiding this comment.
The name is util just like we discussed.
[Comment for reviewer]: I have also moved the packages under the apache/ namespace so that the license difference is visual in the directory structure. I am not sure that is the best way to organize it.
cjdcordeiro
left a comment
cjdcordeiro
left a comment
There was a problem hiding this comment.
Thanks for taking care of the licenses and adding the CI ;) it lgtm from a structural point of view
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Thank you. Probably final details:
| // Validate checks that the Manifest is valid. Note that to do that it has to | ||
| // load practically the whole manifest into memory and unmarshall all the | ||
| // entries. | ||
| func Validate(manifest *Manifest) (err error) { |
There was a problem hiding this comment.
Why are we exposing this right now? We may get in trouble by making this public without considering the details of cross-version support. Right now all this function cares about is "Can the chisel bin handle this file?", and today that's trivial because there's literally zero features from Chisel that allow reading it on request, so lots of uncertainty. Once we make this logic public, though, people are free to link against their own code, and this function will need to do a good job of reporting about all past and future versions of the manifest, which it currently does not.
This issue makes me wonder whether we're missing something else. Before this PR goes in, can we please have a printout by godoc of the public/ packages in some pastebin, so we can have a quick review on the API alone?
There was a problem hiding this comment.
We discussed the exports in the meeting but probably we went over them too fast. This is the pastebin: https://pastebin.canonical.com/p/cDwhTmqpvr/.
In the case of the Validate is was thinking about it in terms of compatibility with the linked current version of the library and to also check for internal consistency. I am not sure I understand what you are saying, but given that we also include the manifest type and fields, isn't there a reasonable expectation that if we change the format old versions of Chisel won't be able to read it anyway?
There was a problem hiding this comment.
Remember the many issues we had to overcome recently about changing chisel.yaml and the consequences for past and future revisions? Now imagine such issues on a file that is actually impossible to update, because there are baked images out there carrying the old format for good. Not only that, but we also have code out there, that we cannot update ourselves, that manipulate such files.
isn't there a reasonable expectation that if we change the format old versions of Chisel won't be able to read it anyway?
Have you reviewed the existing code with the mindset that it must not trust on details that could break future tweaking of the file? It's quite possible that it's totally okay, but I haven't at least. Let me draw a trivial analogy: imagine a JSON parser - depending on how you build it, it's possible you can break the parser without breaking the format. One is a specification, the other is an algorithm. Does that make more sense?
There was a problem hiding this comment.
One is a specification, the other is an algorithm
That made the point clear, I get it now.
| Package string | ||
| Name string | ||
| Essential []SliceKey | ||
| Essential []SetupKey |
There was a problem hiding this comment.
What is a "setup key"? This used to be the key for a package slice, thus SliceKey.
There was a problem hiding this comment.
I thought you had decided that it was a better name when you used it in your previous comment :). I see now that it was a typo.
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Thanks for all the changes.
References:
Relevant excerpts: