Template fails dotnet restore after updating to dotnet core 2 preview 2

[mlorbetske]

From @mike-morr on July 5, 2017 18:1

Steps to reproduce

Install dotnet core
Install dotnet core 2 Preview 1
dotnet new -i Fable.Template
dotnet new fable -n TestProj
dotnet restore (should work)
Install dotnet core 2 Preview 2
dotnet new -i Fable.Template
dotnet new fable -n TestProj
dotnet restore

Expected behavior

dotnet restore succeeds the after installing Preview 2

Actual behavior

An error message is thrown:

  '""' is not recognized as an internal or external command,
  operable program or batch file.
C:\code\TestProj\.paket\Paket.Restore.targets(18,5): error MSB3073: The command """ restore --project "C:\code\TestProj\TestProj.fsproj" --target-framework netstandard1.6" exited with code 9009. [C:\code\TestProj\TestProj.fsproj]

Workaround

./.paket/paket.exe install after this, dotnet restore works successfully.

More Info

It appears like the paket restore targets are getting mangled somehow.

The source for the template is located here: https://github.com/fable-compiler/Fable/tree/master/src/templates/simple

Environment data

dotnet --info output:

.NET Command Line Tools (2.0.0-preview2-006497)

Product Information:
 Version:            2.0.0-preview2-006497
 Commit SHA-1 hash:  06a2093335

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.15063
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.0.0-preview2-006497\

Microsoft .NET Core Shared Framework Host

  Version  : 2.0.0-preview2-25407-01
  Build    : 40c565230930ead58a50719c0ec799df77bddee9

Copied from original issue: dotnet/cli#7078

[mlorbetske]

From @livarcocc on July 5, 2017 18:52

I don't see how this belongs in the CLI repo. We do not own the Fable templates. Should you move this bug to the fable repo?

[mlorbetske]

From @forki on July 5, 2017 18:57

No the problem is that the template source is fine. The template update process is somehow broken.
We have this behavior seen with multiple users.

[mlorbetske]

From @livarcocc on July 5, 2017 18:57

We do not own this Paket.Restore.targets target. We do not know what it does.

[mlorbetske]

From @livarcocc on July 5, 2017 18:59

I see this in the targets:

<PaketCommand Condition=" '$(OS)' == 'Windows_NT'">"$(PaketExePath)"</PaketCommand>
    <PaketCommand Condition=" '$(OS)' != 'Windows_NT' ">$(MonoPath) --runtime=v4.0.30319 "$(PaketExePath)"</PaketCommand>

It is up to the template owners to figure out why this is not being set properly anymore.

@KevinRansom I wonder if this is due to preview2 moving to the new F# SDK?

[mlorbetske]

From @forki on July 5, 2017 19:7

Yes we do understand that you don't own these files. As far as we already know these files are all correct in the template source. And this has nothing to do with F# sdk or paket.
The problem is simply that after dotnet new we have a diff between the fresh project and the original template.
Something is messing this up.

[mlorbetske]

From @KevinRansom on July 5, 2017 21:23

@forki
Hey mate, I think your .targets may be slightly broken:

In the section bellow:

I believe PacketRootPath should be PacketToolsPath or vice-versa.

    <PaketToolsPath>$(MSBuildThisFileDirectory)</PaketToolsPath>
   <PaketExePath Condition=" '$(PaketExePath)' == '' AND Exists('$(PaketRootPath)paket.exe')">$(PaketRootPath)paket.exe</PaketExePath>

  <PropertyGroup>
    <!-- Mark that this target file has been loaded.  -->
    <IsPaketRestoreTargetsFileLoaded>true</IsPaketRestoreTargetsFileLoaded>
    <PaketToolsPath>$(MSBuildThisFileDirectory)</PaketToolsPath>
    <MonoPath Condition="'$(MonoPath)' == '' And Exists('/Library/Frameworks/Mono.framework/Commands/mono')">/Library/Frameworks/Mono.framework/Commands/mono</MonoPath>
    <MonoPath Condition="'$(MonoPath)' == ''">mono</MonoPath>
    <!-- Paket command -->
    <PaketExePath Condition=" '$(PaketExePath)' == '' AND Exists('$(PaketRootPath)paket.exe')">$(PaketRootPath)paket.exe</PaketExePath>
    <PaketCommand>"$(PaketExePath)"</PaketCommand>
    <PaketBootStrapperExePath Condition=" '$(PaketBootStrapperExePath)' == '' AND Exists('$(PaketRootPath)paket.bootstrapper.exe')">$(PaketRootPath)paket.bootstrapper.exe</PaketBootStrapperExePath>
    <PaketBootStrapperCommand>"$(PaketBootStrapperExePath)"</PaketBootStrapperCommand>
  </PropertyGroup>

Edit when I set the path explicity, everything works as expected.

C:\temp\a>set PaketExePath=C:\temp\a\.paket\paket.exe
C:\temp\a>dotnet restore
  Paket version 5.4.5
  packages folder is locked by paket.exe (PID = 16320). Waiting...
  packages folder is locked by paket.exe (PID = 16320). Waiting...
  packages folder is locked by paket.exe (PID = 16320). Waiting...
  Performance:
   - Disk IO: 244 milliseconds
   - Runtime: 21 seconds
  Restoring packages for C:\temp\a\a.fsproj...
  Restoring packages for C:\temp\a\a.fsproj...
  Installing FSharp.Compiler.Tools 4.1.17.
  Installing FSharp.NET.Sdk 1.0.5.
  Generating MSBuild file C:\temp\a\obj\a.fsproj.nuget.g.props.
  Generating MSBuild file C:\temp\a\obj\a.fsproj.nuget.g.targets.
  Restore completed in 4.53 sec for C:\temp\a\a.fsproj.
  Installing Microsoft.DiaSymReader.PortablePdb 1.2.0.
  Installing FSharp.Compiler.Service 13.0.0.
  Installing Newtonsoft.Json 10.0.3.
  Installing Fable.Compiler 1.1.7.
  Installing dotnet-fable 1.1.7.
  Restore completed in 6.07 sec for C:\temp\a\a.fsproj.

C:\temp\a>

[mlorbetske]

From @forki on July 6, 2017 6:32

@KevinRansom root path and tool path are two different things. The problem is not if paket can be called or not (that is just the symptom). Our issue is that we see dotnet new creating a wrong file.
We expect it to exactly create https://github.com/fable-compiler/Fable/blob/master/src/templates/simple/Content/.paket/Paket.Restore.targets . For this issue here, it doesn't matter if the file itself is actually bug free or not. It just matters that we don't get the the file as specified in the template. (I saw it with other files as well, but this was what reproed for multiple users)

Again we think this has nothing to do with paket, f# or fable. It's just a matter of dotnet templating

[mlorbetske]

From @livarcocc on July 6, 2017 6:36

@mlorbetske can you take a look at this? have you heard about something like this before.

@forki, so, the Paket.Restore.targets is not being generated at all? Because from @mike-morr initial bug report, it seemed like it did, but a property was missing, like @KevinRansom suggested. Are we sure we are talking about one single issue here?

[mlorbetske]

From @forki on July 6, 2017 6:44

Yes @mike-morr and myself discussed on gitter before we raised it. It's been investigated over couple of weeks. And since didn't find a solution we raised it here.
The file gets generated, but it has a diff to the uploaded version. Nobody knows why. That's the problem we see.

[mlorbetske]

From @forki on July 6, 2017 6:48

@KevinRansom can please diff your local version with the online version? The fact that path is wrong for you is probably due to the fact that the template creation bug reproed on your machine.

[mlorbetske]

From @livarcocc on July 6, 2017 7:18

Has the version with the problem ever been published? Did it even exist in your repo during some point in time?

[mlorbetske]

From @forki on July 6, 2017 7:22

I think it does not. There are 2 versions of that file. And IIRC my file after dotnet new was a mix of both.

[mlorbetske]

From @livarcocc on July 6, 2017 7:24

Ok, given that this seems like an issue with templates, I will let @mlorbetske investigate. if you want, go ahead and move this issue to dotnet/templating.

[mlorbetske]

@seancpeters can you take a look please? It looks like those targets files should be marked as copy only so that they don't get processed, we should provide instructions on how to do that

[seancpeters]

@mike-morr the problem appears to be caused because during template creation, the default behavior is to process each file from the template source - to allow for optional / variable content in the output. Part of the processing identifies & evaluates conditional statements within each file, which may cause some content to be modified or excluded.

But the default behavior can be overridden by configuration so that a file is simply copied, as opposed to being processed (and potentially modified). This is accomplished by adding the following to the template.json:

  "sources": [
    {
      "modifiers": [
        {
          "copyOnly": [ ".paket/Paket.Restore.targets" ]
        }
      ]
    }
  ]

Note that the "copyOnly" value is an array, and so if there are other files which should just be copied, they can also be listed here.

This is discussed in more detail at https://blogs.msdn.microsoft.com/dotnet/2017/04/02/how-to-create-your-own-templates-for-dotnet-new/ under the "Add optional content" heading.

Please let us know if this takes care of your issue.

[forki]

My question would be: what in this file can lead to such damage? How can we make sure that other files are not messed up?

This is also a much more general question than just for fable templates. The process needs to be transparent in order to be trusted.

[mike-morr]

@seancpeters Thank you for the response! I have the same question as @forki

[seancpeters]

@forki @mike-morr
Unfortunately, we've found a bug causing what I suggested above to not work - until the bug fix is available (#1030). But there is another way around the problem. The template engine implements a way to turn off processing for sections of a file, which only requires adding the following to the file(s) in question, in the correct places. The first flag in each pair below turns off conditional processing, the second one turns it back on.

1. With this version, the flags will be written to the created templates:

<!--/-:cnd-->
<!--/+:cnd-->

2. These will not be written to the created templates:

<!--/-:cnd:noEmit-->
<!--/+:cnd:noEmit-->

Otherwise they behave the same as each other.

Since it sounds as if you'd like your files to remain fully unmodified, it would make sense to put these near the start and end of the relevant files, something like this:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <!--/-:cnd:noEmit-->
  ...
  ...
  <!--/+:cnd:noEmit-->
</Project>

---

Regarding what is causing the target files to be different than the source files:

The template engine has built-in support for conditional processing for many file types, and allows for custom setup for file types which aren't supported - and one can even override the built-in configurations as desired. For msbuild style files (*.*proj, *.*proj.user, *.msbuild, *.targets, and *.props) there is built in support for two ways to setup conditionals.

One of these ways is to use the same conditional syntax as msbuild uses. The template engine decides whether to evaluate the conditional, or if it should be left alone and written to the target file without processing. Some of the conditions in your template caused the engine to choose incorrectly. We're working on a fix for this issue. As an example of using this style, the following file is part of a template currently maintained in this repo: https://github.com/dotnet/templating/blob/rel/vs2017/3-Preview3/template_feed/Microsoft.DotNet.Web.ProjectTemplates.2.0/content/WebApi-CSharp/Company.WebApplication1.csproj

The other way looks like this (I can elaborate on this style if desired):

<!--#if (IndividualLocalAuth && UseLocalDB) -->
  <ItemGroup>
    <None Update="Company.WebApplication1.db" CopyToOutputDirectory="PreserveNewest" />
  </ItemGroup>
<!--#endif-->

Please let me know if this made it clear what's actually going on, or if more clarification is needed. Our development work often gets ahead of the documentation.

[mlorbetske]

There's a second bug here that, despite the test that verifies it doesn't happen passing, unknown tokens are being accepted in string literals by the condition evaluator in msbuild files. So far I've managed to track this down to a condition where the trie evaluation driver needs to read past the end of the token (in this case $() to determine that it is the end of a matched token (because there's a longer token that starts with that sequence). This works as expected & the token is returned - except the buffer position isn't being reset quite correctly. In the compatibility shim introduced to avoid a massive refactor when the new trie that could handle lookarounds was introduced, the length of the discovered token is compared to the change in the buffer position to make sure that the token is anchored at the buffer start (it's important that this happens as bytes could otherwise be skipped over in locating the next known token) - the buffer discrepancy causes this check to fail, indicating that the token that was found isn't acceptable at the current time. When the scan for the token at the current buffer position fails, the head byte of the buffer is appended to (if there is one) the currently active literal and the search for the next token resumes - this adds the $ from the expression start to the buffer, causing the detection of that segment as a token to be missed and the rest of the text of the property to be treated as a literal, the buffer position is then advanced to the location of the ( character & the scanning continues. Once the condition has been fully parsed in this way, the resulting tree contains a comparison of two strings - in this case it's looking to compare the literal $(PaketExePath) to empty string. Since these strings obviously aren't equal, the expression evaluates to false and the block isn't emitted. I have a fix in progress but don't yet know if I'll get it in tonight.

That said, I'll echo @seancpeters's statement - if the file is intended to not be processed, it should be added to the set of copy-only files, speeding up the template generation time and ensuring that no evaluation is done on the file. Due to the current bug in the Include/Copy-Only/Exclude chaining (#1030 has been merged, so it'll be fixed in the upcoming release), the only option to get this working at this time is to turn off conditional processing for the file using the <!--/-:cnd:noEmit--> syntax in the file itself.

[forki]

Thank you for detailed explanation. We were worried we see unexplainable behavior. But this makes sense. We're looking forward to a fix and will apply the workarounds to the various templates. Thank you!

[mlorbetske]

Both fixes will be included in the next release of the 2.0 CLI, closing this issue. Thanks for bringing this to our attention :)

[forki]

Wow. Cool. Thanks

[mike-morr]

Thank you for the quick turnaround. I really appreciate it!