dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.96k stars 4.65k forks source link

Add ResXResourceWriter and associated types #28253

Closed SparkieLabs closed 4 years ago

SparkieLabs commented 5 years ago

ResXResourceWriter is currrently being added to the WinForms SDK, see https://github.com/dotnet/winforms/issues/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 commented 5 years ago

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

danmoseley commented 5 years ago

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.

zsd4yr commented 5 years ago

How are we feeling about this?

danmoseley commented 5 years ago

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?

danmoseley commented 5 years ago

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

danmoseley commented 5 years ago

@stephentoub @jkotas thoughts/objections?

jkotas commented 5 years ago

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.

danmoseley commented 5 years ago

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 commented 5 years ago

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.

danmoseley commented 5 years ago

@nguerrera ?

SparkieLabs commented 5 years ago

"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 commented 5 years ago

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.

SparkieLabs commented 5 years ago

https://docs.microsoft.com/en-us/xamarin/xamarin-forms/app-fundamentals/localization/ https://docs.microsoft.com/en-us/dotnet/api/xamarin.forms.imagesource.fromresource?view=xamarin-forms

nguerrera commented 5 years ago

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

danmoseley commented 5 years ago

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 commented 5 years ago

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

nguerrera commented 5 years ago

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 commented 5 years ago

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:

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

nguerrera commented 5 years ago

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.

tarekgh commented 5 years ago

are we waiting for @rainersigwald investigation to move forward here? I am just trying to see what is the next action item with this issue.

danmoseley commented 5 years ago

@tarekgh I'll put you on the email..

danmoseley commented 5 years ago

If MSBuild decides to take it don't start with my branch verbatim - more changes going in to winforms since. https://github.com/dotnet/winforms/pull/212

danmoseley commented 5 years ago

@sparkie108 to answer your original question, we would like to avoid putting these into CoreFX for reasons that @jkotas mentioned. If you want to read/write ResX without Winforms (eg., on Unix) then I recommend you take the sources - my branch above may be useful.

As you see we are leaning towards taking the same approach in MSBuild for the same reason. That would be best tracked over there.