Fixes to help make VisualFSharp.sln more usable #4555

dsyme · 2018-03-16T02:43:32Z

The most important thing is <EnableDefaultEmbeddedResourceItems>false</EnableDefaultEmbeddedResourceItems> in VisualFSharp.UnitTests.fsproj. The other changes use more normal ways to specify the resources, and ensure we restore test projects with build vs

With this was able to

msbuild VisualFSharp.sln
build VisualFSharp.sln from VS
F5 launch into VisualFSharpFull.vsix.

However this doesn't solve the endless rebuild of FSHarp.Editor, which really kills the usability of VisualFSharp.sln when hitting F5

@brettfo please take a look.

dsyme · 2018-03-16T02:46:45Z

vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj


  <ItemGroup>
-    <EmbeddedResource Update="FSharp.Editor.resx">
+    <EmbeddedResource Include="FSharp.Editor.resx">


@brettfo I'm pretty sure this is normal, I think you were only using Update to avoid EnableDefaultEmbeddedResourceItems?

Correct, I was using Update because $(EnableDefault...) was true. What's the benefit to specifying it this way, e.g., to not allowing the **/*.resx glob from running? The default behavior for SDK projects is to auto-include all *.resx files into the @(EmbeddedResource) item collection. I was trying to stay in line with that so that if another *.resx file is added to this directory that it's automatically included in the group so we don't have to manually manage the files in the project.

brettfo · 2018-03-16T03:30:40Z

vsintegration/fsharp-vsintegration-unittests-build.proj

  <Import Project="..\src\root.traversal.targets"/>

  <Target Name="Restore">
+    <MSBuild Projects="@(ProjectFiles)" Targets="Restore" />


If you're going to restore everything then remove all of the <SdkProjects> lines; those only existed to restore a subset of the projects.

agreed we don't need this

brettfo · 2018-03-16T03:33:54Z

vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj


  <ItemGroup>
-    <EmbeddedResource Update="FSharp.Editor.resx">
+    <EmbeddedResource Include="FSharp.Editor.resx">


Correct, I was using Update because $(EnableDefault...) was true. What's the benefit to specifying it this way, e.g., to not allowing the **/*.resx glob from running? The default behavior for SDK projects is to auto-include all *.resx files into the @(EmbeddedResource) item collection. I was trying to stay in line with that so that if another *.resx file is added to this directory that it's automatically included in the group so we don't have to manually manage the files in the project.

brettfo · 2018-03-16T03:34:49Z

vsintegration/src/FSharp.LanguageService.Base/FSharp.LanguageService.Base.csproj


  <ItemGroup>
-    <EmbeddedResource Update="Microsoft.VisualStudio.Package.LanguageService.resx">
+    <EmbeddedResource Include="Microsoft.VisualStudio.Package.LanguageService.resx">


Same Update vs. Include comment as above.

As mentioned below, the default globbing was incorrectly picking up resources from a project in MockTypeProviders subfolder after you built those projects

brettfo · 2018-03-16T03:35:03Z

vsintegration/src/FSharp.LanguageService/FSharp.LanguageService.fsproj

  <ItemGroup>
    <!-- VSPackage.resx already included -->
-    <EmbeddedResource Update="FSLangSvcStrings.resx">
+    <EmbeddedResource Include="FSLangSvcStrings.resx">


brettfo · 2018-03-16T03:36:25Z

vsintegration/tests/Salsa/VisualFSharp.Salsa.fsproj

    <NoWarn>$(NoWarn);45;47;52;58;75</NoWarn>
    <DisableImplicitFSharpCoreReference>true</DisableImplicitFSharpCoreReference>
    <UsePackageTargetFallbackHack>true</UsePackageTargetFallbackHack>
+    <EnableDefaultEmbeddedResourceItems>false</EnableDefaultEmbeddedResourceItems>


What's the benefit, particularly with this project and VisualFSharp.UnitTests where there are no *.resx files? If the **/*.resx glob is breaking incremental build then that's a larger issue we should address at the SDK level.

@brettfo It wasn't the incremental build (that is still broken) - it was that VisualFSharp.UnitTests project has non-SDK projects in the MockTypeProviders sub-directories which put resources in obj/Debug directories when built, and the implicit glob was incorrectly picking those up, but only after a build

I didn't know about the obj\Debug issue. I see that you rearranged the directories which I like. Only one final nit-pick; instead of decorating each project with <EnableDefault...>false</> can you add it to an appropriate upstream Directory.Build.props or FSharp.Directory.Build.props? That way we won't forget to add it to any new projects in those subdirectories.

KevinRansom · 2018-03-16T03:38:48Z

@brettfo I had the same question, but came down on the side of meh ... at least this way it is explicit.

dsyme · 2018-03-16T12:22:51Z

Re Update or Include and implict v. explicit. I think I don't mind either way. I couldn't find any MSBuild documentation for Update= and had no good mental model for how it worked in combination with globbing (while avoiding duplicates) so assumed we were using some internal thing. And I needed to use EnableDefaultEmbeddedResourceItems in one project because the implicit resource glob was incorrect for "VisualFSharp.UnitTests" (incorrectly getting a completely unrelated resource from nested project MockTypeProviders), so I basically no longer trusted implicit globbing, since it had lost me quite a few hours already.

Are we actually relying on the implicit glob anywhere?

…to buildfixes

…nitTests

dsyme · 2018-03-16T12:54:28Z

@brettfo @KevinRansom I've also permanently removed the use of nested projects:

vsintegration/tests/unittests/MockTypeProviders --> vsintegration/tests/MockTypeProviders
vsintegration/tests/unittests --> vsintegration/tests/UnitTests so it no longer looks like a directory name you might nest further projects in.

Those renames/moves are enough to solve the problem

dsyme · 2018-03-19T14:39:16Z

@brettfo I'll merge this if that's ok - I've mentioned above why moving to implicit globbing was a problem for us

dsyme · 2018-03-19T17:42:02Z

@brettfo Sorry, I didn't see this comment for some reason:

Only one final nit-pick; instead of decorating each project with <EnableDefault...>false</> can you add it to an appropriate upstream Directory.Build.props or FSharp.Directory.Build.props? That way we won't forget to add it to any new projects in those subdirectories.

Agreed we should do that

* fixes for VisualFSharp.sln usage * fixes for VisualFSharp.sln usage * Update FSharpEmbedResourceText.fs * move MockTypeProviders * rename vsintegration/tests/unittests --> vsintegration/VisualFSharp.UnitTests * rename again * fix paths

dsyme and others added 5 commits March 15, 2018 23:14

Merge branch 'build-in-vs' of https://github.com/brettfo/visualfsharp

24b7c0a

Merge branch 'master' of http://github.com/Microsoft/visualfsharp

eb87631

fixes for VisualFSharp.sln usage

8156daf

fixes for VisualFSharp.sln usage

4b828b0

Update FSharpEmbedResourceText.fs

4e688ee

dsyme commented Mar 16, 2018

View reviewed changes

dsyme mentioned this pull request Mar 16, 2018

Warnings during build-from-VisualStudio #4541

Closed

brettfo suggested changes Mar 16, 2018

View reviewed changes

KevinRansom approved these changes Mar 16, 2018

View reviewed changes

dsyme added 3 commits March 16, 2018 12:45

move MockTypeProviders

18e1758

Merge branch 'buildfixes' of https://github.com/dsyme/visualfsharp in…

c9ddadd

…to buildfixes

rename vsintegration/tests/unittests --> vsintegration/VisualFSharp.U…

23c6b15

…nitTests

dsyme added 3 commits March 16, 2018 12:59

rename again

edc5bd5

integrate master

f08c483

fix paths

cc63539

dsyme merged commit 4df9975 into dotnet:master Mar 19, 2018

Fixes to help make VisualFSharp.sln more usable #4555

Fixes to help make VisualFSharp.sln more usable #4555

Uh oh!

Conversation

dsyme commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinRansom commented Mar 16, 2018

Uh oh!

dsyme commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented Mar 19, 2018

Uh oh!

dsyme commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dsyme commented Mar 16, 2018 •

edited

Loading

dsyme Mar 16, 2018 •

edited

Loading

dsyme commented Mar 16, 2018 •

edited

Loading

dsyme commented Mar 16, 2018 •

edited

Loading