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

[BinFmt] ValueType and ValueTypeArray #8923

Closed
JanKrivanek opened this issue Jun 20, 2023 · 0 comments · Fixed by #9111
Closed

[BinFmt] ValueType and ValueTypeArray #8923

JanKrivanek opened this issue Jun 20, 2023 · 0 comments · Fixed by #9111

Comments

@JanKrivanek
Copy link
Member

Background

#6215
This subitem is focused on https://github.com/dotnet/msbuild/blob/main/src/Shared/TaskParameter.cs#L282

Suggested approach

We should be able to de/serialize without BinaryFormatter (Blittable types just be grabing the bytes, nonblittable by marshaling. Or possibly just use marshalling on all types).
Carefull unit tests should be added - for blittable and nonblittable types (https://learn.microsoft.com/en-us/dotnet/framework/interop/blittable-and-non-blittable-types)

@ladipro ladipro self-assigned this Aug 1, 2023
ladipro added a commit that referenced this issue Aug 14, 2023
Fixes #8923 

### Context

Moving `TaskParameter` instances between processes uses `TranslateDotNet` for serializing/deserializing value types and value type arrays. This functionality is implemented on top of `BinaryFormatter` and as such problematic from compliance perspective.

### Changes Made

Reworked `TaskParameter` so now it operates on the following types:

- `PrimitiveType` and `PrimitiveTypeArray`, which are converted with `Convert.ChangeType(o, typeof(T))` to/from string or serialized natively if the type is supported by `ITranslator`. These types additionally use `System.TypeCode` of the specific type as a discriminator.
- `ValueType` and `ValueTypeArray`, which are converted only to string because they can only be used in task outputs. The requirement for these to implement `IConvertible` is not new. Without the changes in this PR a serializable but non-`IConvertible` task output parameter can be moved from the task host process but fails later because the same `Convert.ChangeType` conversion is attempted by the engine.
- `ITaskItem, ITaskItemArray, Invalid, Null` - no change, same behavior as before.

### Testing

Existing unit tests (extended, enhanced, and refactored).

### Notes

- The new code is under a change wave check so it can be disabled if needed.
- If the user disables the 17.8 change wave, they will have to make sure that MSBuild can call `BinaryFormatter`. This is an unfortunate limitation but it seems to be done elsewhere already (`TranslateException` for example) so I'm assuming it's been thought though.
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants