Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Jul 23, 2020

This eases a development pain where the use of multi-targeting for FSHarp.COmpiler.Private badly stresses our VS tooling while working on the compiler itself. It makes it easier for developers to locally use only a netstandard2.0 FSharp.Compiler.Private.dll ,for compiler work/testing etc, giving much faster IDE support.

This is a benign change

  1. it doesn't change the product

  2. It is not activated by default (the comment in FSHarp.Compiler.Private.fsproj indicates how to activate it.)

  3. doesn't actually try to use .NET Standard 2.0 dll any more than it's used so far

  4. when you enable it you can't actually build a full working VSIX but I've verified you can build the compiler and run individual unit tests, right the way up to the VIsual Studio unit test

#7959 contains some further fixes which @cartermp found for packaging etc.

Notes:

  • For IndentedTextWriter, the necessary code is in .NET Standard 2.0 to activate this. This change will also include this functionality in FCS

  • The other change moves the "grab these extra DLLs for VS setup" logic into fsc project

@dsyme dsyme changed the title indented text writer is in netstandard2.0 allow use of only netstandard2.0 FSharp.Compiler.Private to ease development in VS Jul 23, 2020
@dsyme dsyme changed the title allow use of only netstandard2.0 FSharp.Compiler.Private to ease development in VS Allow use of only netstandard2.0 FSharp.Compiler.Private to ease development in VS Jul 23, 2020
@dsyme
Copy link
Contributor Author

dsyme commented Jul 23, 2020

See also #7959

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jul 23, 2020

but the comment in FSHarp.Compiler.Private.fsproj indicates how to activate it.

If this indeed makes working in FSharp.sln and VisualFSharp.sln (much) less of a pain and hopefully eliminates the crashes of VS or even the sudden disappearance without warning, that would be absolutely awesome 💯

Can we place that comment in the Contribution.md file as well? I doubt people will inspect the fsproj file regularly, let alone when they play around the first time.

@dsyme
Copy link
Contributor Author

dsyme commented Jul 23, 2020

@brettfo One thing I noticed was that System.Reflection.TypeExtensions.dll was missing from the list of DLLs to package with fsc.exe and fsi.exe - it is present in their output directory so I assume we need it?

@dsyme
Copy link
Contributor Author

dsyme commented Jul 23, 2020

Can we place that comment in the Contribution.md file as well? I doubt people will inspect the fsproj file regularly, let alone when they play around the first time.

Not yet - while this PR is useful it's not ready for common scenario yet, too many tests fail

<Tailcalls>true</Tailcalls> <!-- .tail annotations always emitted for this binary, even in debug mode -->
</PropertyGroup>

<Target Name="CopyToBuiltBin" BeforeTargets="BuiltProjectOutputGroup" AfterTargets="CoreCompile">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This target needs to stay in this file because this is where the package references live. It's simply an implementation detail that they also end up next to fsc in the output and if that ever changes we'll be in for a world of hurt trying to figure out what changed.

I'm guessing this changed because building only netstandard2.0 here means not all of the files are present, e.g., Microsoft.Build.dll. If that's the case, those BuildProjectOutputGroupKeyOutput items could be made to behave better with Condition="'$(TargetFramework)' == 'net472'"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i see thanks (out of curiosity could you also put package references in FSC?)

folder "InstallDir:Common7\IDE\CommonExtensions\Microsoft\FSharp"
file source="$(BinariesFolder)\fsc\$(Configuration)\$(TargetFramework)\fsc.exe" vs.file.ngen=yes vs.file.ngenArchitecture=All vs.file.ngenPriority=2 vs.file.ngenApplication="[installDir]\Common7\IDE\CommonExtensions\Microsoft\FSharp\fsc.exe"
file source="$(BinariesFolder)\fsc\$(Configuration)\$(TargetFramework)\fsc.exe.config"
file source="$(BinariesFolder)\fsc\$(Configuration)\$(TargetFramework)\Microsoft.Build.dll"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather these references come specifically from the project output dir that originally requested them, e.g., FSharp.Compiler.Private for most.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I see

file source="$(BinariesFolder)\FSharp.Compiler.Private\$(Configuration)\$(TargetFramework)\System.Resources.Extensions.dll"
file source="$(BinariesFolder)\FSharp.Compiler.Private\$(Configuration)\$(TargetFramework)\System.Runtime.CompilerServices.Unsafe.dll"
file source="$(BinariesFolder)\FSharp.Compiler.Private\$(Configuration)\$(TargetFramework)\System.Threading.Tasks.Dataflow.dll"
file source="$(BinariesFolder)\FSharp.Compiler.Server.Shared\$(Configuration)\$(TargetFramework)\FSharp.Compiler.Server.Shared.dll" vs.file.ngen=yes vs.file.ngenArchitecture=All vs.file.ngenPriority=2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one specifically should remain.

@KevinRansom
Copy link
Contributor

This is on my list to do properly.

@dsyme
Copy link
Contributor Author

dsyme commented Jul 24, 2020

This is on my list to do properly.

fab

@dsyme
Copy link
Contributor Author

dsyme commented Jul 24, 2020

I'll close this as I'm not yet regularly using it (got a bunch of test failures related to this in a test I needed to actually run for the string interpolation work)

@dsyme dsyme closed this Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants