Final fix #3113 #7024

matthid · 2019-06-20T19:43:08Z

This ports the relevant code from Roslyn to fix #3113. In particular

This change is conservative and only uses the managed implementation when the old one is not available. We could probably remove the old one if we want and get rid of the FX_NO_LINKEDRESOURCES define.

The ported code is quite ugly because I used cs2fs as a first sketch and then fixed some of the issues. Also, I ported more code than we actually need, we can discuss if we remove it or want to use it in the future.

I tested a bit with netframework and the netcoreapp version but I guess we should see what the test-suite says.

EDIT:
This also solves:

…nto the codebase

Conflicts: src/absil/ilsupp.fs src/absil/ilwrite.fs src/buildfromsource/FSharp.Compiler.Private/FSharp.Compiler.Private.fsproj

matthid · 2019-06-21T00:54:54Z

By the way something I noticed while debugging (not sure why it happened, but probably because the Debugger triggered some side-effect when displaying values):

And indeed we don't set UseShellExecute, so it defaults to true on .NET framework, which is most likely not what we want here:

fsharp/src/absil/ilsupp.fs

Lines 665 to 669 in 74c6e22

    
           let mutable psi = System.Diagnostics.ProcessStartInfo cvtres 
        
           psi.Arguments <- cmdLineArgs 
        
           psi.CreateNoWindow <- true ; // REVIEW: For some reason, this still creates a window unless WindowStyle is set to hidden 
        
           psi.WindowStyle <- System.Diagnostics.ProcessWindowStyle.Hidden 
        
           let p = System.Diagnostics.Process.Start psi

Given the REVIEW comment in the above code we probably should just set it to true, which - if we do not remove this code anyway - allows us to remove the CreateNoWindow and WindowStyle "hacks"?

KevinRansom · 2019-06-22T09:39:13Z

@matthid there is work going on in the dotnetsdk to perform manage resource serialization for more than strings. When that is ready, I was intending to convert to using that, rather than porting cvtres to the F# compiler.

matthid · 2019-06-22T09:46:11Z

@KevinRansom I guess you talk about dotnet/roslyn#11386 (But I'm not sure) or where is that work tracked?

matthid · 2019-06-22T09:55:34Z

Just to clarify cvtres.cs is not really useful for us anyway: We only really use the first 100 lines (I could delete the rest). This is because we already have the stuff in the res format. It is a bit ironic that I had to add code to parse the byte sequences we generated ourself just some lines before. This whole code path looks quite ugly and it uses byte[][] everywhere but that is another topic.

So what we really need is NativeResourceWriter.cs, but given that it turned out to be only around another 180 lines of code I'm not sure if it is worth waiting for another year to have this solved?

cartermp · 2019-06-22T15:42:24Z

I'd much rather that we do something like this in the interim, as realistically no movement on anything other than shipping .NET Core 3.0 is going to be happening for the SDK between now and October.

TIHan · 2019-06-27T04:26:59Z

I can see why we went down this road. There isn't any other way to write those resources on the coreclr compiler and the current bug is annoying.

I need to think about this. We want to fix this issue. The implementation is self-contained, not much API surface area, though I can already tell we can do some cleanup.

What @KevinRansom said, if dotnetsdk adds the support, then we could leverage that.

It's too bad System.Reflection.Metadata doesn't handle native and managed resources yet. @tmat, how much work would be involved in getting them integrated with System.Reflection.Metadata? I imagine a lot, but I'm just wondering. It would be much more impactful to do the work and expose it via SRM. This was also one of the last remaining pieces of the metadata re-write that I was doing too.

While this feature is important, it's not of upmost importance versus all of the other work we are dealing with. So, I need think about it as an interim solution.

matthid · 2019-06-27T07:28:19Z

It would be much more impactful to do the work and expose it via SRM.

Question is why it hasn't been done this way in the first place.

I can already tell we can do some cleanup.

Like I said above. Also note that we could probably do a lot of cleanup in the compiler internals here, as currently byte[] is used everywhere. But we should keep this PR minimal for now and cleanup once SRM got the new feature...

TIHan · 2019-06-27T19:59:49Z

Question is why it hasn't been done this way in the first place.

Time and priorities like everything else. Personally, if we were tasked for doing this work for F#, I would probably do the work for SRM and F# would just use SRM.

tmat · 2019-06-27T20:13:19Z

how much work would be involved in getting them integrated with System.Reflection.Metadata? I imagine a lot, but I'm just wondering. It would be much more impactful to do the work and expose it via SRM.

That would definitely be useful in many ways. unfortunately, I won't have a chance to look into this anytime soon. Since SRM is general purpose reader/writer and part of Core CLR that a lot of tools depend on it has very high bar on design and implementation quality and performance. So, yes, it seems like a significant effort.

matthid · 2019-06-27T20:48:01Z

Time and priorities like everything else. Personally, if we were tasked for doing this work for F#, I would probably do the work for SRM and F# would just use SRM.

Yes that's what I thought, question is why this would be different for us? Do we have a higher/different quality bar for the F# compiler?
It feels like there is some general issue here. Maybe we can move that out of this PR?

KevinRansom · 2019-07-12T18:38:59Z

@matthid,

csharp, never did this inside the compiler, it relied on the build running cvtres.exe and embedding the generated resx file.
In common with C# and VB for the core clr, F# supported string resource generation only.
Now that development is occurring to enable C# to a wider range of resource serializations on the coreclr, which is necessary because V# and VB have designer scenarios for UI development. We will leverage the same work for C#.
We are very unlikely to add to F# a second mechanism that will need maintaining that enables the F# compiler to do this for itself.
The quality bar for F# and C# is similar.

matthid · 2019-07-12T18:54:07Z

csharp, never did this inside the compiler, it relied on the build running cvtres.exe and embedding the generated resx file.

Maybe pre-roslyn times? In fact, Roslyn is where I got the code from.

In common with C# and VB for the core clr, F# supported string resource generation only.

Again I don't understand: C# works, F# doesn't

Now that development is occurring to enable C# to a wider range of resource serializations on the coreclr, which is necessary because V# and VB have designer scenarios for UI development. We will leverage the same work for C#.
We are very unlikely to add to F# a second mechanism that will need maintaining that enables the F# compiler to do this for itself.

So as I read this we wait?

nguerrera · 2019-08-11T21:27:32Z

@KevinRansom There seems to be some misunderstanding in this discussion.

Now that development is occurring to enable C# to a wider range of resource serializations on the coreclr, which is necessary because C# and VB have designer scenarios for UI development.

That work and this issue are completely separate. The resource in question here for file version info is a Win32 resource, and the non-string resources that have been brought up in 3.0 are inside managed manifest resources (.resources generated from .resx). There's no shared code between these two cases and nothing about bringing the non-string managed resources up on .NET Core will help F# to emit the Win32 resource for file version info.

It would be nice if System.Reflection.Metadata had an API for this as discussed above, but that will indeed take some time to get the API designed and approved. In the meantime, I don't see a problem with porting the code that Roslyn uses for this.

cartermp · 2019-08-21T23:36:35Z

Also note: lack of this seems to block #7220, meaning we cannot have a single target for FSharp.Compiler.Private until this issue gets resolved

matthid · 2019-08-22T18:07:39Z

@cartermp Like I said we should just get over it, clean it up and merge it. It solves a real-life problem.

I'm even happy to finish/rework this once I see a chance of this being merged.

Given the comment of @nguerrera it seems like we all don't even have the same understanding of what happens and what doesn't happen in the eco-system to make this PR obsolete. At the very least it would be nice to have technical clarifications on what happens where and links to the work items.

cartermp · 2019-08-22T21:24:02Z

@matthid I'm thinking of waiting for @KevinRansom to get back (he took the week off to do cool stuff with family). But it seems that based on what @nguerrera is saying, this particular issue will not be addressed by anything the SDK will be doing in the future, so it's highly likely that we'll need this work.

So we'll have to figure out how we want to proceed, but by all accounts this is also blocking two internal issues two:

Can't go .NET Standard only for FSharp.Compiler.Private, thus making the dev experience in the compiler awful
Can't move towards a reality where we only deploy one compiler, like what Roslyn does

So in addition to fixing #3113 this can also simplify stuff for the repo as a whole. I definitely want to see this get in.

KevinRansom

Since the NetSdk aren't doing this after all, we certainly do it. Sorry for the earlier confusion.

It is quite amusing to me that this was originally C++ code that got translated to C# and then to F#. The notion of using a translation program, hadn't occurred to me.

Thanks this is nice work.

Kevin

KevinRansom · 2019-08-26T18:21:42Z

src/absil/writenativeres.fs

+open System.Diagnostics
+open System.IO
+open System.Reflection.Metadata
+//open Roslyn.Utilities;


Not necessary.

KevinRansom · 2019-08-26T18:22:37Z

src/absil/writenativeres.fs

+                | _ -> ()
+                i <- i + 1
+        size
+    (*


I assume this commented out code is not necessary, or is it a reminder for something?

KevinRansom · 2019-08-26T18:22:56Z

src/absil/writenativeres.fsi

+    static member SortResources : resources : IEnumerable<Win32Resource> -> IEnumerable<Win32Resource>
+    static member SerializeWin32Resources : builder:BlobBuilder * theResources:IEnumerable<Win32Resource> * resourcesRva:int -> unit
+    (*
+    static member SerializeWin32Resources(builder : BlobBuilder, resourceSections : ResourceSection, resourcesRva : int) -> unit


I assume this commented out code is not necessary, or is it a reminder for something?

Indeed. All commented out code can just be removed.

KevinRansom · 2019-08-26T18:25:06Z

src/absil/ilsupp.fs

+        linkNativeResourcesManaged unlinkedResources ulLinkedResourceBaseRVA fileType outputFilePath
+    else
+#endif
+#if !FX_NO_LINKEDRESOURCES


Let's lose the old spawn cvtres.exe method, the new code should work on desktop just fine, any issues that appear are just bugs that should be found.

Yes

This change is conservative and only uses the managed implementation when the old one is not available. We could probably remove the old one if we want and get rid of the FX_NO_LINKEDRESOURCES define.

I'm happy to get rid of it, I just decided to be more conservative first.

KevinRansom · 2019-08-26T18:26:35Z

vsintegration/Utils/LanguageServiceProfiling/Options.fs

@@ -77,6 +77,10 @@ let FCS (repositoryDir: string) : Options =
             @"src\absil\ilprint.fs"
             @"src\absil\ilmorph.fsi"
             @"src\absil\ilmorph.fs"
+             @"src\absil\writenativeres.fsi"
+             @"src\absil\writenativeres.fs"
+             @"src\absil\cvtres.fsi"


Is there any reason not to Jam cvtres and writenativeres into the same file. They really are the same feature.

No, I decided to keep old structure (which made porting just a bit easier), but otherwise, there is no real reason to split this.

KevinRansom · 2019-08-26T18:30:50Z

@MattiD, let me know if you want to take care of the issues, otherwise, I will merge this, and take care of them myself.

Once again, thanks for this.

Kevin

matthid · 2019-08-26T18:50:50Z

let me know if you want to take care of the issues, otherwise, I will merge this, and take care of them myself.

@KevinRansom I'm happy either way. So feel free to finish this (as it might take a couple of days for me to finish). Just one note: A lot more stuff can be deleted if we care about "minimal" change. See this comment

Just to clarify cvtres.cs is not really useful for us anyway: We only really use the first 100 lines (I could delete the rest). This is because we already have the stuff in the res format. It is a bit ironic that I had to add code to parse the byte sequences we generated ourself just some lines before.

matthid · 2019-08-26T18:53:47Z

It is quite amusing to me that this was originally C++ code that got translated to C# and then to F#. The notion of using a translation program, hadn't occurred to me.

Indeed, even more amusing:

It is a bit ironic that I had to add code to parse the byte sequences we generated ourself just some lines before.

I'm pretty sure that whole part could be a lot cleaner (and probably faster), but this code-path likely doesn't affect perf in any meaningful way.

KevinRansom · 2019-08-26T19:17:12Z

@MattiD, Okay, I will take care of it then.

Thanks for this

matthid · 2019-08-26T19:20:41Z

@KevinRansom also feel free to use my GitHub handle with an additional "h": "matthid". I'm subscribed anyway but just in case :)

…nto the codebase (dotnet#7024)

matthid added 2 commits June 20, 2019 21:30

Fix dotnet#3113 by porting the relevant roslyn codeand integrate it i…

da881d8

…nto the codebase

Merge branch 'master' of github.com:dotnet/fsharp into final_fix_3113

74c6e22

Conflicts: src/absil/ilsupp.fs src/absil/ilwrite.fs src/buildfromsource/FSharp.Compiler.Private/FSharp.Compiler.Private.fsproj

cartermp requested review from KevinRansom and dsyme June 21, 2019 18:07

matthid closed this Jul 12, 2019

nguerrera mentioned this pull request Aug 11, 2019

Generate F# assemblyinfo in new fsproj #3113

Closed

matthid reopened this Aug 22, 2019

KevinRansom approved these changes Aug 26, 2019

View reviewed changes

KevinRansom merged commit c22fa0b into dotnet:master Aug 26, 2019

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021

Fix dotnet#3113 by porting the relevant roslyn codeand integrate it i…

f225f12

…nto the codebase (dotnet#7024)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Final fix #3113 #7024

Final fix #3113 #7024

matthid commented Jun 20, 2019 •

edited

Loading

matthid commented Jun 21, 2019

KevinRansom commented Jun 22, 2019

matthid commented Jun 22, 2019

matthid commented Jun 22, 2019

cartermp commented Jun 22, 2019 •

edited

Loading

TIHan commented Jun 27, 2019 •

edited

Loading

matthid commented Jun 27, 2019 •

edited

Loading

TIHan commented Jun 27, 2019

tmat commented Jun 27, 2019

matthid commented Jun 27, 2019

KevinRansom commented Jul 12, 2019

matthid commented Jul 12, 2019

nguerrera commented Aug 11, 2019 •

edited

Loading

cartermp commented Aug 21, 2019

matthid commented Aug 22, 2019

cartermp commented Aug 22, 2019

KevinRansom left a comment

KevinRansom Aug 26, 2019

KevinRansom Aug 26, 2019

KevinRansom Aug 26, 2019

matthid Aug 26, 2019

KevinRansom Aug 26, 2019

matthid Aug 26, 2019

KevinRansom Aug 26, 2019

matthid Aug 26, 2019

KevinRansom commented Aug 26, 2019

matthid commented Aug 26, 2019

matthid commented Aug 26, 2019

KevinRansom commented Aug 26, 2019

matthid commented Aug 26, 2019

Final fix #3113 #7024

Final fix #3113 #7024

Conversation

matthid commented Jun 20, 2019 • edited Loading

matthid commented Jun 21, 2019

KevinRansom commented Jun 22, 2019

matthid commented Jun 22, 2019

matthid commented Jun 22, 2019

cartermp commented Jun 22, 2019 • edited Loading

TIHan commented Jun 27, 2019 • edited Loading

matthid commented Jun 27, 2019 • edited Loading

TIHan commented Jun 27, 2019

tmat commented Jun 27, 2019

matthid commented Jun 27, 2019

KevinRansom commented Jul 12, 2019

matthid commented Jul 12, 2019

nguerrera commented Aug 11, 2019 • edited Loading

cartermp commented Aug 21, 2019

matthid commented Aug 22, 2019

cartermp commented Aug 22, 2019

KevinRansom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinRansom commented Aug 26, 2019

matthid commented Aug 26, 2019

matthid commented Aug 26, 2019

KevinRansom commented Aug 26, 2019

matthid commented Aug 26, 2019

matthid commented Jun 20, 2019 •

edited

Loading

cartermp commented Jun 22, 2019 •

edited

Loading

TIHan commented Jun 27, 2019 •

edited

Loading

matthid commented Jun 27, 2019 •

edited

Loading

nguerrera commented Aug 11, 2019 •

edited

Loading