Skip to content
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

Adds registry metadata #1060

Merged
merged 17 commits into from Jul 5, 2023
Merged

Conversation

silesmo
Copy link
Member

@silesmo silesmo commented Jun 10, 2023

Adds support for registry metadata as discussed: bytecodealliance/registry#116

@calvinrp
Copy link

Couple suggestions / questions:

  • add the command so that it can be used from wasm-tools CLI, e.g. wasm-tools registry-metadata print (not sure about the naming, maybe package show and package set?);
  • there is a metadata command already, should we consider putting it under that command? probably not but asking the question
  • currently need to call with args for the input path, e.g. registry-metadata-cli -p some.wasm print; can we also do registry-metadata-cli print some.wasm?
  • add an option to provide a JSON file input or STDIN? (probably minifying the JSON before putting into the custom section)
  • maybe add test / example cases with links being exercised?

@alexcrichton
Copy link
Member

Thanks for this! I'd second all of what @calvinrp said as well. For organization it'd be best to put the new executable under src/bin/wasm-tools/*.rs, ideally in the metadata.rs file by augmenting that existing comment. This ideally would prevent duplicating the loops to read/write from the existing wasm-metadata crate as well. (possibly merging the crates together too)

@silesmo
Copy link
Member Author

silesmo commented Jun 14, 2023

Thanks for this! I'd second all of what @calvinrp said as well. For organization it'd be best to put the new executable under src/bin/wasm-tools/*.rs, ideally in the metadata.rs file by augmenting that existing comment. This ideally would prevent duplicating the loops to read/write from the existing wasm-metadata crate as well. (possibly merging the crates together too)

Yes I agree with this. I was hesitant to do this initially since I wasn't sure we even wanted this in the wasm-tools repo. But if we are happy with that when I will make those changes!

@silesmo
Copy link
Member Author

silesmo commented Jun 14, 2023

Couple suggestions / questions:

* add the command so that it can be used from `wasm-tools` CLI, e.g. `wasm-tools registry-metadata print` (not sure about the naming, maybe `package show` and `package set`?);

* there is a `metadata` command already, should we consider putting it under that command? probably not but asking the question

* currently need to call with args for the input path, e.g. `registry-metadata-cli -p some.wasm print`; can we also do `registry-metadata-cli print some.wasm`?

* add an option to provide a JSON file input or STDIN? (probably minifying the JSON before putting into the custom section)

* maybe add test / example cases with `links` being exercised?

Thanks for the suggestions. I will go ahead and make the suggested changes.

@alexcrichton
Copy link
Member

Oh to be clear I'm not sure if this is the best place for this to live either. I can provide feedback on how to integrate this once it's decided to integrate here, but I'd leave that up to other folks to make the judgement call of whether this belongs here.

@silesmo
Copy link
Member Author

silesmo commented Jun 14, 2023

I will bring it up in the wasm-registry sig meeting later today and we will see after that.

@esoterra
Copy link
Contributor

We discussed this topic yesterday and reached a consensus around putting it in the wasm-tools repo so that it can be read from and written to using wasm-encoder/decoder more easily.

One way to think about including this is that it's a not-completely-standard custom section and as long as we call that out, I think it's fine to have support for it in wasm-tools. This isn't that different from encoding/decoding DWARF into custom sections for debugging tools. If needed, we can even hide it behind a feature flag to make it only visible/usable by people who want it.

@calvinrp
Copy link

Looks good to me. Thank you!

@silesmo
Copy link
Member Author

silesmo commented Jun 29, 2023

This should be ready to be reviewed now.
@alexcrichton or @Kylebrown9 if you have the time to take a look

@silesmo silesmo changed the title Add lib and simple cli wrapper for registry metadata Adds registry metadata Jun 29, 2023
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me! One minor question about what to do in the appending case, but otherwise seems reasonable to me to add

crates/wasm-metadata/src/lib.rs Show resolved Hide resolved
@alexcrichton alexcrichton merged commit e92e311 into bytecodealliance:main Jul 5, 2023
14 checks passed
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 5, 2023
Handles new features that are required through bytecodealliance#1060
alexcrichton added a commit that referenced this pull request Jul 5, 2023
Handles new features that are required through #1060
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 11, 2023
Releases recent changes such as:

* More support for the gc proposal. bytecodealliance#1045 bytecodealliance#1059
* Support for resources throughout most tooling. bytecodealliance#1053 bytecodealliance#1068 bytecodealliance#1070 bytecodealliance#1082
  bytecodealliance#1079 bytecodealliance#1083 bytecodealliance#1084 bytecodealliance#1105 bytecodealliance#1113 bytecodealliance#1116
* Support for `include` in WIT files. bytecodealliance#1054 bytecodealliance#1085 bytecodealliance#1088
* WIT worlds may now be rejected if the same interface can be "reached"
  as both an import and an export simultaneously. bytecodealliance#1081 bytecodealliance#1107
* Support for registry metadata in `wasm-tools metadata`. bytecodealliance#1060
* A new subcommand `wasm-tools component targets`. bytecodealliance#1089
* An `--out-dir` argument is supported on `wasm-tools component wit` to
  print the entire `Resolve` instead of just one package. bytecodealliance#1108
* Miscellaneous bug fixes and improvements. bytecodealliance#1061 bytecodealliance#1065 bytecodealliance#1074 bytecodealliance#1073 bytecodealliance#1078 bytecodealliance#1077
  bytecodealliance#1072 bytecodealliance#1086 bytecodealliance#1091 bytecodealliance#1094 bytecodealliance#1114 bytecodealliance#1106
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 11, 2023
Releases recent changes such as:

* More support for the gc proposal.
  [bytecodealliance#1045](bytecodealliance#1045)
  [bytecodealliance#1059](bytecodealliance#1059)
* Support for resources throughout most tooling.
  [bytecodealliance#1053](bytecodealliance#1053)
  [bytecodealliance#1068](bytecodealliance#1068)
  [bytecodealliance#1070](bytecodealliance#1070)
  [bytecodealliance#1082](bytecodealliance#1082)
  [bytecodealliance#1079](bytecodealliance#1079)
  [bytecodealliance#1083](bytecodealliance#1083)
  [bytecodealliance#1084](bytecodealliance#1084)
  [bytecodealliance#1105](bytecodealliance#1105)
  [bytecodealliance#1113](bytecodealliance#1113)
  [bytecodealliance#1116](bytecodealliance#1116)
* Support for `include` in WIT files.
  [bytecodealliance#1054](bytecodealliance#1054)
  [bytecodealliance#1085](bytecodealliance#1085)
  [bytecodealliance#1088](bytecodealliance#1088)
* WIT worlds may now be rejected if the same interface can be "reached"
  as both an import and an export simultaneously.
  [bytecodealliance#1081](bytecodealliance#1081)
  [bytecodealliance#1107](bytecodealliance#1107)
* Support for registry metadata in `wasm-tools metadata`.
  [bytecodealliance#1060](bytecodealliance#1060)
* A new subcommand `wasm-tools component targets`.
  [bytecodealliance#1089](bytecodealliance#1089)
* An `--out-dir` argument is supported on `wasm-tools component wit` to
  print the entire `Resolve` instead of just one package.
  [bytecodealliance#1108](bytecodealliance#1108)
* Miscellaneous bug fixes and improvements.
  [bytecodealliance#1061](bytecodealliance#1061)
  [bytecodealliance#1065](bytecodealliance#1065)
  [bytecodealliance#1074](bytecodealliance#1074)
  [bytecodealliance#1073](bytecodealliance#1073)
  [bytecodealliance#1078](bytecodealliance#1078)
  [bytecodealliance#1077](bytecodealliance#1077)
  [bytecodealliance#1072](bytecodealliance#1072)
  [bytecodealliance#1086](bytecodealliance#1086)
  [bytecodealliance#1091](bytecodealliance#1091)
  [bytecodealliance#1094](bytecodealliance#1094)
  [bytecodealliance#1114](bytecodealliance#1114)
  [bytecodealliance#1106](bytecodealliance#1106)
alexcrichton added a commit that referenced this pull request Jul 11, 2023
Releases recent changes such as:

* More support for the gc proposal.
  [#1045](#1045)
  [#1059](#1059)
* Support for resources throughout most tooling.
  [#1053](#1053)
  [#1068](#1068)
  [#1070](#1070)
  [#1082](#1082)
  [#1079](#1079)
  [#1083](#1083)
  [#1084](#1084)
  [#1105](#1105)
  [#1113](#1113)
  [#1116](#1116)
* Support for `include` in WIT files.
  [#1054](#1054)
  [#1085](#1085)
  [#1088](#1088)
* WIT worlds may now be rejected if the same interface can be "reached"
  as both an import and an export simultaneously.
  [#1081](#1081)
  [#1107](#1107)
* Support for registry metadata in `wasm-tools metadata`.
  [#1060](#1060)
* A new subcommand `wasm-tools component targets`.
  [#1089](#1089)
* An `--out-dir` argument is supported on `wasm-tools component wit` to
  print the entire `Resolve` instead of just one package.
  [#1108](#1108)
* Miscellaneous bug fixes and improvements.
  [#1061](#1061)
  [#1065](#1065)
  [#1074](#1074)
  [#1073](#1073)
  [#1078](#1078)
  [#1077](#1077)
  [#1072](#1072)
  [#1086](#1086)
  [#1091](#1091)
  [#1094](#1094)
  [#1114](#1114)
  [#1106](#1106)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants