sifive / wit

Workspace Integration Tool
Apache License 2.0
23 stars 13 forks source link

Use same json format for lock/manifest/workspace #242

Closed mmjconolly closed 4 years ago

mmjconolly commented 4 years ago

I've produced a few revisions that I don't like, so just putting one up so we can talk about it

The idea is that all the json formats could be the same (commit 1), and that only one area of the codebase knows how to serialize the output and write the files (commit 2).

It makes the presumption that one format is good enough for the 3 use cases we have. This does change the format used for wit-lock.json but I feel that's ok as its not typically committed to revision control like wit-manifest.json.

I haven't tried to refactor manifest.py/lock.py/dependancy.py/package.py too much, only enough to use RepoEntry.

Longer term I feel this helps us add more fields, like branch name and remote name.

jackkoenig commented 4 years ago

I'm generally in favor of the idea. One worry I had with changing the formats in the past is compatibility. We're pre-1.0 so obviously we don't have to preserve backwards nor forwards compatibility but it might be nice to create a schema and version the format so that we have the option to provide backwards compatibility and we can give "forwards compatibility" warnings or errors when seeing a schema version that is too new.

jackkoenig commented 4 years ago

To clarify, I could be convinced that that is not necessary for this change, I just think it might be worth considering since we're revisiting the format anyway.

I think we also do at least want to read the old format for a while if possible, even if we start outputting the new format.

richardxia commented 4 years ago

I'm also generally in favor of this change. Regarding backwards compatibility, if it's not too difficult to do so and if it doesn't make the code too messy, I'd also support doing that, at least for one intermediate version (wit v0.14?). However, I agree with @mmjconolly's statement that wit-lock.json is generally not committed to version control today, so even if we do change the format, it will only cause a breakage if you switch wit versions after initializing a workspace.

In general, I think it would be good to structure our code such that the serialization/deserialization of the import file formats is in its own module like @mmjconolly has here, and if we do this right, we can even have serializers/deserializers for multiple different versions of the file format at the same time if their internal structures are wildly different.

If we are going to break backwards compatibility, I'd like to suggest something that we sneak into the file format while we're at it: change the top-level data structure in the file from an array to an object/dictionary. The current structure we have with the array at the top level prevents us from storing metadata about the whole file or adding a new type of encoded data besides the repository specification. If we added a new object structure as the top level, we can still put the repositories under a key, but then we can add stuff like a "manifest version" or other useful pieces of information.

E.g.

{
  "manifestVersion": "1.0.0",  // Version of the file format, not of the wit tool
  "dependencies": [
    {
      "commit": "deadbeef",
      "name": "api-scala-sifive",
      "source": "git@github.com:sifive/api-scala-sifive.git"
    }
  ],
  "devDependencies": []
}
mmjconolly commented 4 years ago

I think if we change the wit-manifest.json format at all (ie, adding a version), we may also break scripts people have in other repositories that directly modify this file

richardxia commented 4 years ago

I'd be OK with breaking scripts other people have written, especially if it gives us the opportunity to finally start versioning the data file format itself. I'd say that the wit manifest file format is "implementation details" at the moment, and the only "supported" way of updating it is via running wit commands.