Skip to content

feat: rework modpkg format#24

Merged
Crauzer merged 19 commits intomainfrom
feat/rework-modpkg-format
Mar 1, 2025
Merged

feat: rework modpkg format#24
Crauzer merged 19 commits intomainfrom
feat/rework-modpkg-format

Conversation

@Crauzer
Copy link
Member

@Crauzer Crauzer commented Nov 11, 2024

  • Create utils functions for modpkg

@Crauzer Crauzer added the enhancement New feature or request label Nov 11, 2024
@Crauzer Crauzer self-assigned this Nov 11, 2024
@Crauzer Crauzer mentioned this pull request Nov 11, 2024
4 tasks
alanpq
alanpq previously approved these changes Nov 12, 2024
@Crauzer
Copy link
Member Author

Crauzer commented Nov 12, 2024

btw I'm probably in favor of doing our own serialization for metadata and strings, there seem to be differences between msgpack libraries in how they handle enums for example, so I'd like to avoid potential issues there

@moonshadow565
Copy link

btw I'm probably in favor of doing our own serialization for metadata and strings, there seem to be differences between msgpack libraries in how they handle enums for example, so I'd like to avoid potential issues there

what about having metadata and strings table being just chunks instead of dedicated fields?

@Crauzer
Copy link
Member Author

Crauzer commented Nov 13, 2024

what about having metadata and strings table being just chunks instead of dedicated fields?

Sure, I don't think it should cause too much trouble for the user

@Crauzer
Copy link
Member Author

Crauzer commented Jan 27, 2025

@moonshadow565 @alanpq starting this up again

@moonshadow565
Copy link

moonshadow565 commented Jan 27, 2025

@moonshadow565 @alanpq starting this up again

Been thinking about it, no need to overcomplicate it.
Just make the format simple as possible (maybe with binrw ?).
For signature checking we can just generate dedicated TOC + sha256 of real metadata that we sign instead.

@alanpq
Copy link
Contributor

alanpq commented Jan 27, 2025

+1 on the binrw/etc usage, since we own both sides we might as well keep it as boilerplate free as possible

@alanpq alanpq force-pushed the feat/rework-modpkg-format branch from d42cd18 to bec2a9b Compare February 7, 2025 11:47
@alanpq
Copy link
Contributor

alanpq commented Feb 7, 2025

@Crauzer @moonshadow565 few breaking changes with the binrw port:

  • Modpkg's metadata, chunk_paths & wad_paths are no longer stored in the chunk map
  • the strings inside of ModpkgLicense are null terminated now instead of len-prefixed (this feels better to me but can easily change it back)
  • there might be more that I've either forgotten or missed, so I recommend poking around yourselves

Non-breaking but I've also added proptest for checking the soundness of size()


impl ModpkgMetadata {
/// The total size of the metadata when written to bytes.
pub fn size(&self) -> usize {

Choose a reason for hiding this comment

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

There should be better way to do this with binrw

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, we either do this or we use some custom String struct that impls BinWrite/Read (or we ask binrw to implement mapping inside an option somehow)

Copy link

@moonshadow565 moonshadow565 Feb 7, 2025

Choose a reason for hiding this comment

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

Why not just implement dummy Write that tracks the size and write metdata into it?

See https://docs.rs/binrw/latest/binrw/io/trait.Write.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Github mobile lied to me, thought this was a comment on the optional_string_write map stuff. There probably is a better way I just ported what already was there - I'll let @Crauzer decide what to replace it with

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I don't even see the point of this function existing now that we're using binrw. I think we can remove it. If needed, we can add it back using some other way as moon said.


impl ModpkgAuthor {
/// The total size of the author when written to bytes.
pub fn size(&self) -> usize {
Copy link

@moonshadow565 moonshadow565 Feb 7, 2025

Choose a reason for hiding this comment

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

Same as ModpkgMetadata::size

@Crauzer Crauzer force-pushed the feat/rework-modpkg-format branch from 26ad6cb to 4e71cc5 Compare March 1, 2025 01:11
@Crauzer
Copy link
Member Author

Crauzer commented Mar 1, 2025

Going to merge this as it is, we can further refine the format as needed while I work on the mod project packer.

@Crauzer Crauzer merged commit b0f2e6d into main Mar 1, 2025
1 check passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in league-mod Mar 1, 2025
@Crauzer Crauzer deleted the feat/rework-modpkg-format branch March 1, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants