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

Add rudimentary support for DeserializingResourceReader #504

Merged
merged 9 commits into from
Jul 15, 2023

Conversation

ElektroKill
Copy link
Contributor

@ElektroKill ElektroKill commented Jun 10, 2023

  • Updated code in CheckReaders to match the actual meaning of the fields. The implementation now matches the one in the runtime.
  • Added several new members to allow creating of more customized resource sets.
  • Added support for the DeserializingResourceReader class introduced in .NET 5 and now in the System.Resources.Extensions package.

The changes should be source and binary compatible with the previous dnlib version. This limited the support I could provide for this new resource reader which specifies additional information on how to deserialize objects with different serialization kinds for the UserType type code.

See:
https://github.com/dotnet/dotnet/blob/95703b57cf3dfaa79f054d277551a809d4f7696a/src/runtime/src/libraries/System.Resources.Extensions/src/System/Resources/Extensions/DeserializingResourceReader.cs#L108

The old ResourceReader always used BinaryFormater while the new code will first read some additional information and then deserialize the object. This new implementation in dnlib leaves the handling of this to the user which could be confusing. Ideally, we'd want to handle that in dnlib itself but that can't be done without introducing breaking changes.

Would it be OK for us to introduce breaking changes as part of a minor release or would this have to be part of a major release? I'd like to use this new code in dnSpyEx and would like to know if I should wait for the next minor release or perhaps, implement a custom version of the ResourceReader resource handling in dnSpyEx temporarily. If breaking changes are OK in this component, I'd also like to extend the writing support to support version 1 resources!

@wtfsck
Copy link
Contributor

wtfsck commented Jul 8, 2023

Would it be OK for us to introduce breaking changes as part of a minor release or would this have to be part of a major release?

It needs to be a new major release. Do you have any other breaking changes you'd like to see?

@ElektroKill
Copy link
Contributor Author

Would it be OK for us to introduce breaking changes as part of a minor release or would this have to be part of a major release?

It needs to be a new major release. Do you have any other breaking changes you'd like to see?

See #509, other than that no potential breaking changes come to mind as of now.

Should I proceed with implementing the breaking changes necessary for the proper support of this new resource reader class in this PR?

@wtfsck
Copy link
Contributor

wtfsck commented Jul 9, 2023

Should I proceed with implementing the breaking changes necessary for the proper support of this new resource reader class in this PR?

Yes, next release will bump the major version number.

@ElektroKill
Copy link
Contributor Author

The two latest commits add proper support for this new resource reader type. Something which is still TBD is writing format version 1 resources.

src/DotNet/Resources/ResourceBinaryWriter.cs Outdated Show resolved Hide resolved
src/DotNet/Resources/ResourceDataFactory.cs Show resolved Hide resolved
src/DotNet/Resources/ResourceReader.cs Show resolved Hide resolved
src/DotNet/Resources/ResourceWriter.cs Show resolved Hide resolved
src/DotNet/Resources/ResourceReader.cs Show resolved Hide resolved
@ElektroKill ElektroKill requested a review from wtfsck July 12, 2023 12:02
@wtfsck wtfsck merged commit 29d4743 into 0xd4d:master Jul 15, 2023
2 checks passed
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.

2 participants