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 ResXResourceWriter and associated types #34204

Open
sparkie108 opened this Issue Dec 21, 2018 · 19 comments

Comments

6 participants
@sparkie108
Copy link

sparkie108 commented Dec 21, 2018

ResXResourceWriter is currrently being added to the WinForms SDK, see dotnet/winforms#216.

Shouldn't ResXResourceWriter and associated types be added to CoreFX instead? It would then be possible to programatically create resx files without a dependency on the WinForms SDK. Almost all of the rest of System.Resources is already part of .NET Standard 2.0.

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Dec 21, 2018

I believe there were reasons to not put it in CoreFX ... @ericstj @danmosemsft might know details

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Dec 21, 2018

We were discussing this earlier in the week with @tmat @merriemcgaw @rainersigwald . We probably need to move it into CoreFX because we don't want to add a new dependency from MSBuild "upwards" to Winforms. Fortunately it looks pretty easy to separate out the 5 or 6 files from Winforms.

If there is general agreement, I can ask someone to go ahead and do this.

@danmosemsft danmosemsft added this to To do in WPF/Winforms support work via automation Dec 21, 2018

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 10, 2019

How are we feeling about this?

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Jan 11, 2019

If it goes to MSBuild, it would need to be a in a standalone assembly so that Winforms could include it reasonably in its shared framework. If it stays in Winforms, it would have to be a in separate assembly so it can be built and packaged for cross platform (which nothing in winforms does). CoreFX does seem likely more reasonable.

To see whether it can be extracted I got the src building successfully:
https://github.com/dotnet/corefx/compare/master...danmosemsft:resx2?expand=1

Work remains -- move this into the right contract so the ref builds successfully also. It needs to be fairly high in the stack as it depends on TypeConverter. System.Windows.Extensions is one possibility although MSBuild needs this cross plat and that package was intended to be Windows specific. We maybe need a new contract (System.CrossPlatform.Extensions ..?)

In separate discussions we would like to avoid using the BinaryFormatter codepaths ourselves. Does that require new API?

@tmat @rainersigwald any reason why this work should not now go ahead in CoreFX?

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Jan 11, 2019

There is also non trivial work to write or port tests for this.

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Jan 11, 2019

@stephentoub @jkotas thoughts/objections?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 11, 2019

I think it is fine for the writer to be in WinForms SDK and for the reader to be WinForms. I do not think it is important to have a first class ResX files, without depending on WinForms. ResX files are pretty WinForms specific. The support for them lived in System.Windows.Forms.dll on .NET Framework, and it is where it should stay.

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Jan 11, 2019

Where is "Winforms SDK" built from - do you mean something different to the dotnet/winforms repo?

If it stays there - MSBuild needs to reference it somehow, and it needs to be built and packaged for all platforms. If that's possible - great, I'm fine with that.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 11, 2019

I am not up to speed on what WinForms SDK is or where it is built from. The WinForms-specific msbuild tasks can always include the source for the writer - this will work without introducing new dependencies even if they are built in msbuild repo itself.

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Jan 11, 2019

@sparkie108

This comment has been minimized.

Copy link

sparkie108 commented Jan 11, 2019

"ResX files are pretty WinForms specific" I disagree. My use case is sharing resources in a platform independent way in a Xamarin Forms app. You can read ResX apps on iOS/Mac fine as it's part of .NET Std, but why can't I generate a ResX file programmatically on a Mac? In VS .NET Core console apps Project Properties have a Resources tab which is ResX-based.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 11, 2019

You can certainly use parts of WinForms or WPF without the rest of these stacks. I think we should be factoring these parts out of WinForms or WPF into CoreFX only if they are good forward-looking technology. The ResX resources are not a particularly great format. I do not think we should be breathing a new life into them by factoring them out into CoreFX repo and making them easier to use without WinForms. If somebody wants to use ResX resources without WinForms, they can always do so by taking the sources.

@nguerrera

This comment has been minimized.

Copy link
Member

nguerrera commented Jan 11, 2019

I am not sure, but "WinForms SDK" may refer to Microsoft.NET.Sdk.WindowsDesktop, which is not yet open source. It's sitting next to WPF sources that haven't been moved to github yet. I believe it will land in dotnet/wpf at some point. It includes almost nothing specific to windows forms. The majority of it are the build tasks for handling WPF xaml.

We have made the call that to use WPF xaml, you need to pull in the WindowsDesktop SDK. We cannot make a clean call for resx embedded resource handling since these are used by every managed project type. Outside winforms, it's commonly used as just a glorified string table. To date, the msbuild task that processes resx on .NET Core version of msbuild was hacked to just assume everything is a string and it just parses the xml with its own code. It is buggy and doesn't error in unsupported cases, but instead interprets serialization of other object types as text.

So we need to move msbuild to code that handles all of the resx cases correctly. We cannot depend on winforms for that, but we can consider copying code.

cc @rainersigwald

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Jan 11, 2019

It is not a large amount of code to copy, and also presumably very stable code (we don't plan to change it). That might indeed be easiest.

@nguerrera

This comment has been minimized.

Copy link
Member

nguerrera commented Jan 11, 2019

I think that would probably be OK if it is indeed small and stable. @livarcocc FYI

@nguerrera

This comment has been minimized.

Copy link
Member

nguerrera commented Jan 11, 2019

Also, just to clarify an earlier point:

we don't want to add a new dependency from MSBuild "upwards" to Winforms.

We can't add this dependency because Winforms does not exist on all platforms that msbuild supports.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 11, 2019

msbuild to code that handles all of the resx cases correctly.

The general resource case as it existed in .NET Framework depends on binary serialization (e.g. look for the BinaryFormatter references in @danmosemsft delta). The binary serialization comes with:

  • Versioning problems: Ideally, you want to use the target assemblies for the binary serialization. The types you want to binary serialize may not even exist in the runtime that the msbuild is running on.
  • Security problems: You are running arbitrary code during the build, not just the compilers your trust.

What is the plan to address these problems? This needs to be understood first before we talk about where the code lives.

@nguerrera

This comment has been minimized.

Copy link
Member

nguerrera commented Jan 11, 2019

Agreed. @rainersigwald has been investigating whether we can convert from .resx -> .resources without deserialize -> reserialize step.

The code would have to change, but we still need some parts of it to interpret things in the resx apart from deserialization of data values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment