Sdk: [dotnet migrate]"bower install " & "dotnet bundle" commands from prepublish scripts section should not be migrated from project.json to csproj

[vijayrkn]

Steps to reproduce

migrate project.json to csproj with prepublish section

Expected behavior

"bower install" &
"dotnet bundle"
is not added to the prepublishscript section in csproj.

Actual behavior

it is getting added.

Environment data

dotnet --info output:

[livarcocc]

This is conflicting with https://github.com/dotnet/cli/issues/4605. Is this something that changed recently?

[vijayrkn]

@livarcocc - You can close https://github.com/dotnet/cli/issues/4605. We don't want "bower install" and "dotnet bundle" added as part of migration (they have been removed from temples as well).

[livarcocc]

@vijayrkn dotnet/sdk#7084 is closed. I fixed it three months ago. This is a change from what was expect then and now. We will take care of it.

But while we are on it. Is there anything else that has changed?

[vijayrkn]

@livarcocc - Initially the plan was to ship this section in the csproj & the PM team changed this requirement (end of November ) and decided to not ship this section in templates. Hence, this new request. Sorry about the confusion.

There are no other changes planned. Thanks for taking care of this.

[tmds]

@vijayrkn won't this break apps which rely on those commands being executed on publish?

[vijayrkn]

@tmds
Feel free to add this section to your app if the app uses bower & bundling.

<Target Name="PrepublishScript" BeforeTargets="PrepareForPublish">
    <Exec Command=”bower install” EnvironmentVariables=”Path=$(ExternalToolsPath)” />
    <Exec Command="dotnet bundle" />
  </Target>

The new templates does not carry these section (by default) because some of these commands may be platform dependent. The default templates are meant to work on all platforms.

[livarcocc]

@vijayrkn is there any chance then that a migrated project will fail because it does not have this section?

[vijayrkn]

If there are commands later in the section that depends on bower being installed from the previous step, they may fail.
But again, if migrate adds this section and if the template is run from a non-windows platform it may again fail.
The reason to remove this is to make sure that templates does not carry any platform specific commands that have better chance of failing on a different platform.

[tmds]

Unfortunately the cli did not check the return values of these scripts before, which means if they were broken, the publish operation would still succeed. The msbuild project will now detect those failures and the publish operation will fail.

It is better to include these commands in the migration and cause a publish to fail than to drop them and cause the publish to succeed when it shouldn't.

@vijayrkn what repo contains the web template?

[vijayrkn]

VS web templates are here - https://github.com/aspnet/templates

[scottaddie]

@vijayrkn Will a warning be included in the migration report within Visual Studio 2017, so that the developer knows the prepublish script wasn't migrated? It's my opinion that most people would expect that script to migrate.

[dasMulli]

This would need to be super specific for apps that have been created from the template and not touch arbitrary bower install scripts (e.g. custom gulp build definitions that rely on a bower for & npm install).

[livarcocc]

I don't think migration should drop these commands. It is fine for the new templates to not have them, but I don't think it is ok for migration to assume that existing projects are not making use of it.

For instance, one of the issues pointed out is that bower install won't work cross-platform. I honestly don't see why not, as long as you have bower available on your path, it will work. Plus, if it was not working in project.json (for whatever reason), it is not the goal of migration to make your csproj now work.

[vijayrkn]

Sure, makes sense. we can add them during migration (even though not part of templates.). When the bower install command is invoked from VS during publish, if they are not in the path, VS publish fails. is it possible to add this property to the path $(ExternalToolsPath) before executing the command ?

[livarcocc]

Which property should be added to ExternalToolsPath? And where would we do that? And what about dotnet bundle?

[scottaddie]

Maybe I'm not following, but shouldn't Bower always be available if the ASP.NET and web development workload is installed? See C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Web\External\bower.cmd. If Bower isn't available on the path, would it be more appropriate to tell the user to modify their VS 2017 installation to include the aforementioned workload?

[vijayrkn]

Bower is installed with ASP.NET and web development workload but it is not added to the path. The msbuild property $(ExternalToolsPath) has the details to the path where it is installed. So, running a command similar to this will execute fine even when it is not added to the path globally.

<Exec Command="SET PATH=$(ExternalToolsPath);%PATH% &amp;&amp; bower install" />

Without this, publish may throw this error. then it is upto the user to add this to the path.

'bower' is not recognized as an internal or external command, operable program or batch file

If we are fine with this, Then we can close this bug.

[vijayrkn]

@mlorbetske

[livarcocc]

I am fine with that.

I believe it is a fine expectation that when the user's project has a dependency on bower then it should be possible to find bower.

Also, is that ExternalToolsPath available through the CLI as well?

[scottaddie]

What about the case where I've configured External Web Tools such that $(PATH) takes precedence over what ships with VS? Here's an example of that:

I might do that if I have a newer version of Bower installed and want to use that instead of the older version that ships with VS. By doing the following, wouldn't this configuration change be ignored?

<Exec Command="SET PATH=$(ExternalToolsPath);%PATH% &amp;&amp; bower install" />

[livarcocc]

Besides, this looks like a VS only construct and not applicable to the CLI. So, for migration, I would rather that we do not set that.

[vijayrkn]

@livarcocc $(ExternalToolsPath) is only available through VS.

@scottaddie You are right. ExternalToolsPath already adds Path in the right order it is defined in VS. So just setting the path to $(ExternalToolsPath) is good enough.

I am fine if we decide not to add this.

If the users run into this issue, they can either add bower to path systemwide. Or if they need the same path settings as in VS, they can add this to the csproj after migration.

    <PropertyGroup>
      <SetExternalToolsPath Condition ="'$(ExternalToolsPath)' != ''">SET PATH=$(ExternalToolsPath) &amp;&amp; </SetExternalToolsPath>
    </PropertyGroup>

    <Exec Command="$(SetExternalToolsPath)bower install" />

[dasMulli]

But SET PATH= will only work on windows. On *nix, there isn't a set binary and on shells like bash, set is a builtin command for shell options, positional parameters etc.

[vijayrkn]

I was not asking to add this to the cli. This is only for VS scenarios where prepublish script needs to access the same external tools path set in VS, then the 'SET PATH' can be added manually in the csproj after migration (*windows only because this is a VS specific scenario).

If the users run into this issue, they can either add bower to path systemwide. Or if they need the same path settings as in VS, they can add this to the csproj after migration.

[livarcocc]

@vijayrkn Migration has no concept of running for VS and running for CLI. Plus, whatever projects it generates, it should work for both. We should not be doing VS only project properties.

[livarcocc]

Ok. I am going to close this issue. If there is anything required for migration, please re-activate.

[vijayrkn]

Sure, We can close this. I will see if I can set the path from VS itself before invoking the VS Publish scenario.

For all CLI scenarios (including Git/Kudu deploy) bower needs to be available in the path if prepublish script section has "bower install"