-
Notifications
You must be signed in to change notification settings - Fork 781
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
Make F# resources work more like C# #3352
Conversation
Given a resource in a folder named `AFolder/AResourceFile.txt` in a project with a root namespace of `fsharpresources`, then the following command line arg should be passed to the compiler ``` --resource:AFolder/AResourceFile.txt,fsharpresources.AFolder.AResourceFile.txt ``` whereas currently it just passes the name of the file. This also fixes passing a `LogicalName` to the resource. Fixes dotnet#1050 && dotnet#922 I looked at how Roslyn was doing this [here](https://github.com/dotnet/roslyn/blob/6847f1e5a909395aae9456e8f366cbf4deb86b69/src/Compilers/Core/MSBuildTask/ManagedCompiler.cs#L739) es. Lines starting
@nosami This has been a huge source of pain historically - really glad you have sorted this out Could you also submit to fsharp/fsharp and we'll check what the extra testing there says (there is testing enabled in that repo that is not enabled here) |
It took me most of today to work that out :) Will submit to fsharp/fsharp... I tested with that repo locally, then applied the PR here :) |
@dsyme what is the nature of the extra testing? |
Fwiw, I ran the Xamarin tests that build each F# template in VS for Mac via msbuild with this change applied. All passed. |
@KevinRansom I think it is
The problems with Linux/OSX testing in this repo are
For now I've personally run out of time w.r.t. on the repo unification work and have to focus on language design and research work. |
@nosami Great to know you have these tests. Resources are used nearly always in UI. Do the tests also run the resulting apps? |
@don Syme<mailto:Don.Syme@microsoft.com>
You aint kidding about the Cambridge and QA test suites. But nothing gets achieved if nothing gets done on them.
|
@dsyme The tests just download nuget packages, check that msbuild works, and that there are no squiggles in the editor - https://github.com/mono/monodevelop/blob/master/main/external/fsharpbinding/MonoDevelop.FSharp.Tests/TemplateTests.fs |
@nosami I'm concerned that the generated names of resources may be wrong - this usually shows up as a runtime failure in some "GetResource" call |
@nosami You have one failure:
you might need to adjust the test, since it looks like
We will also need guidance about whether this is a breaking change (for Mono and/or Windows) |
Well, the generated names may well be different now, this is definitely a breaking change. But, I think it's a good breaking change - it looks as if it's never worked correctly. I think that the test is wrong and needs to be changed to match the code. The new names look correct to me. |
As far as mono goes, this change is needed. Some things in Xamarin iOS just plain don't work https://bugzilla.xamarin.com/show_bug.cgi?id=57689 being a prime example. I would rather have that work and take the fallout from the change. |
Yes, I'm inclined to believe that. I don't have access to that specific bug though @cartermp and @KevinRansom will need to make the call about the breaking change. Are the scenarios simply
Is there anything we could do to moderate the breaking change? e.g. only activate it if some other setting is passed in, and set that in the Xamarin projects and all future templates? |
The main issue is that you can't have the same filename in multiple folders. This happens frequently for Xamarin, e.g. the same icon but in different sizes - they get put in subfolders but the filename is the same. It would be far too difficult to try and work against that with F# specific hacks. |
I totally agree, I want to see this fixed once-and-for-all. But can we make this conditionally activated in some way by a |
is this actually breaking? Was using folders ever officially supported? It definitely never felt like it was. |
@matthid Yes, I believe you could do it and inspect the assembly for the resource names generated. Also this now respects |
Do you have a bugzilla account? I thought this was a public bug As this is quite risky (even though I feel it's right), I could apply this patch to mono only. That way only mono / xamarin users would be fixed... but then Windows users would still be affected I feel like that the correct solution would be to have a switch that reverted this behaviour. |
I think we want this fix to apply for Xamarin-on-Visual-Studio-for-Windows too. Inconsistency in Xamarin tooling between Windows and OSX and Linux would be bad
Are you sure we can't activate it explicitly? That would be the safest route. We don't want existing F# code breaking. |
It's interesting that this is not a compiler fix, but instead a fix in the build task modification just changing what's passed to the compiler. It seems that we could and should enable this as the default approach. And add a buildtask option to use the old behavior to enable as a quick fix for anyone whose app breaks. I don't think there is a lot of risk of making this change, if we have a simple fallback. Kevin |
For sure, we can do this. But it feels (to me) like adding a switch to make it behave like it should behave is wrong - there should be a switch to enable the opposite. |
@nosami Could you also review these notes please? This is where I documented all the variations in behaviour a while back. At the time I didn't know how to fix the problem at root cause. Also those notes don't cover RootNamespace. (The behaviour as documented there was when using XBuild on Linux - XBuild caused havoc, and we are at last ridding ourselves of that, which is I think what's allowing us to make progress on this at last. We should check again with MSBuild on Linux) Perhaps write up a new version of all those scenarios too, and we need to check behaviour on both Windows and OSX.
This is not right. Do we really want upgrading to VS2017 15.4 or the latest Xamarin package to just give random "missing resource" failures on startup of applications? Silently changing the names of generated resources has busted, among other things, the compiler packages and apps on multiple occasions in the past.
In both cases the failures only manifest at runtime, and sometimes we detected the problem only once we upgraded to use the (broken) compiler package. I absolutely want this problem to cease, but I'm 100% convinced that this has the potential to be a risky breaking change to compilations that use resources - including the two cases where I have used them - since most people end up putting resources in directories sooner or later - and most project files have RootNamespace set silently. We would only consider this safe if we were 99.9% sure that no one was doing that - but we know that they are. We were nervous about fixing this blindly when we first came across the whacky behaviour (and unimplemented RootNamespace behaviour) in F# 3.0 timeframe. If we were nervous then, we should be even more careful now. I really want this fixed, but we should
If we release this change then I'm fairly confident it will silently break multiple instances of existing enterprise code (resources tend to get used in enterprises more than data-scripting/ETL code). In that case we are really in the proverbial, as we will likely have to revert to old behaviour, and then we will have even more confusion about what the actual behaviour is. (I really don't want to be in a situation where @KevinRansom, @Pilchie, @cartermp, myself and others at Microsoft have to hot-patch the F# compiler when MrBigBank calls Microsoft and says that their apps are now crashing on startup now they've upgraded to latest VS2017...) |
@nosami Is the RootNamespace part of the fix necessary here? My understanding from your comment above is that it isn't? BTW the steps in this bug indicate all that's required for a user to have done for her to be faced with a breaking change here:
That is a very common scenario. Many people will have embedded resources into their applications or libraries at some point. They will now get a silent "resource not found" or null pointer exception at runtime if they recompile. And VFPT has been allowing folder creation in Visual Studio since late 2014, and Xamarin/MonoDevelop have always allowed it. I hate the fact that this was botched in F# 2.0 - and we have to dig ourselves out of the hole. But silently breaking existing code isn't the way out. |
@dsyme: You know the risks far better than I do :) I know that this is super risky. Whatever you decide is fine with me :) |
@nosami The only thing I can think of is a new setting
(or some similar property) which we put in F# VS and Xamarin templates for old-style project files, and we always activate it for new-style project files. Any chance you could you draft that? |
@dsyme |
Super. |
Being able to have multi dpi images in F# Android Applications is a must really, with resources being broken for so long its difficulty to recommend F# for android at all. |
Yes, it must be fixed |
With ``` <UseStandardResourceNames>true</UseStandardResourceNames> ``` we get the following output ``` --resource:AFolder/AResourceFile.txt,fsharpresource.AFolder.AResourceFile.txt ``` With the value omitted or set to false, the behaviour is as it was previously. ``` --resource:AFolder/AResourceFile.txt ```
OK. I've added a With
we get the following output
With the value omitted or set to false, the behaviour is as it was previously.
|
@@ -89,24 +89,28 @@ this file. | |||
|
|||
<CreateFSharpManifestResourceName | |||
Condition="'@(ResxWithNoCulture)' != '' AND '$(UsingXBuild)' == 'true'" | |||
UseStandardResourceNames="$(UseStandardResourceNames)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KevinRansom Does anything need to be done for .NET SDK targets here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsyme. I will update VS templates and the dotnet sdk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KevinRansom Is this fix in the latest .NET SDK or will it be in a future release? I cannot get it to work on my project, which looks like this:
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>netcoreapp2.0</TargetFramework>
<RootNamespace>Generators</RootNamespace>
<UseStandardResourceNames>true</UseStandardResourceNames>
</PropertyGroup>
<ItemGroup>
<Compile Include="Program.fs" />
</ItemGroup>
<ItemGroup>
<EmbeddedResource Include="Templates\*.liquid" />
</ItemGroup>
If I use ildasm
to look at the manifest, the resource doesn't have the prepended namespace:
.mresource public _TestClass.liquid
@nosami Great. Could you also adjust that test that was failing to run in both modes and check each? And check that we have RootNamespace=null as a variation on the test too? |
Sure - will do. |
Writing the tests might not be so easy - I only have OSX on this laptop and VisualFSharp doesn't compile there. I can try and do it "blind". Or wait until next week when I am back on my main machine. |
@@ -89,24 +89,28 @@ this file. | |||
|
|||
<CreateFSharpManifestResourceName | |||
Condition="'@(ResxWithNoCulture)' != '' AND '$(UsingXBuild)' == 'true'" | |||
UseStandardResourceNames="$(UseStandardResourceNames)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsyme. I will update VS templates and the dotnet sdk.
... to generate resource identifiers the same way that C# does. This allows having the same resource name in different folders. Follow on commit to dotnet/fsharp#3352 (comment)
Given a resource in a folder named
AFolder/AResourceFile.txt
in a project witha root namespace of
fsharpresources
, then the following command line argshould be passed to the compiler
whereas currently it just passes the name of the file. This also fixes passing
a
LogicalName
to the resource.Fixes #1050 &&
#922
I looked at how Roslyn was doing this here