Add posh support. #33
Conversation
|
Nice, we have been thinking about supporting PowerShell! Eventually, we might make this the default and give up on .cmd completely. Would you be able to add basic smoke test coverage to test/smokeTest.js? Thanks! |
|
I added posh scenario to smoke test and passed all. (and added missing node.js) |
|
Great, thanks! It may take us until next week to get to reviewing this as we're in the middle of a release, but it's definitely a change that we want and will look at soon :) |
| function selectNodeVersion() { | ||
| if ($env:KUDU_SELECT_NODE_VERSION_CMD -ne $null) { | ||
| # The following are done only on Windows Azure Websites environment | ||
| & $env:KUDU_SELECT_NODE_VERSION_CMD "$DEPLOYMENT_SOURCE{SitePath}" "$DEPLOYMENT_TARGET" "$DEPLOYMENT_TEMP" |
There was a problem hiding this comment.
so after testing this, I found out that & $env:KUDU_SELECT_NODE_VERSION_CMD doesn't work because the value of $env:KUDU_SELECT_NODE_VERSION_CMD is set by Kudu as node "D:\...\Kudu.Services.Web\bin\scripts\selectNodeVersion" and you can't run that with & in powershell. I'm not sure if there is a way to run a variable $var in powershell if
$var = "app.exe `"path to something`""
There was a problem hiding this comment.
Invoke-Expression $var
However, it's considered dangerous.
|
I went over the code change and it looks fine to me, thanks for the PR @shibayan 👍 |
|
Thank you for feedback @ahmelsayed |
|
The Invoke-Expression with an May i suggest building the call along the lines of: $env:SOME_CMD = 'echo'
$DEPLOYMENT_SOURCE = "C:\deploy source\file.txt"
$DEPLOYMENT_TARGET = "C:\deploy dest\file.txt"
[string]$NodeCmd = $env:SOME_CMD
[array]$arguments = $DEPLOYMENT_SOURCE, $DEPLOYMENT_TARGET
& $NodeCmd $($arguments.ForEach({ $_ -replace '^|$','"' }) -join ' ')Output: Maybe even a regex match on |
|
@snobu is your concern primarily about attack vectors? On Azure, it's typically not an issue as you need to have full power over the site in order to use any Kudu functionality. i.e. if you have the power to inject an evil environment variable, you can just as easily replace the custom deployment script to do evil directly. But in the end, you can only damage your own site. |
|
You're right it's not a vulnerability. If someone other than the site owner can change $envs, using or not using iex is like closing the gates after the horses have left the stables (in the words of Lee Holmes) Anyhow I guess I liked my quote-on-the-fly .ForEach too much not to offer it as an alternative :) |
|
I just tried it with a .NET app (https://github.com/KuduApps/Dev14_Net46_Mvc5/blob/powershell/deploy.ps1), and it failed for me when git pushing. The error is a bit puzzling, referring to "Window title cannot be longer than 1023 characters": Interestingly, when I retried from portal, it worked. But then the next git push failed. I think this may be due to small environmental differences between the git push case (which uses kudu.exe), and the retry/CI case that doesn't. Thoughts? |
|
I found that changing: to Fixes the issue. But I'm not good with PowerShell, so I'm not sure if it's the correct thing to do. In fact, I don't understand the failure. Reopening PR since it seems broken as it is. |
|
Dot sourcing.
By dot sourcing a script you still have the vars in your PowerShell session after the script terminates. |
|
Thanks @snobu. I was able to repro by reducing the script to a one-liner with just the nuget command. And the fact that it fails from git push but works in other scenarios is quite puzzling as well. |
|
Try these two:
|
|
@snobu it should be
So we now have 3 candidate fixes! :) |
|
Disregard my last comment. The error is ABOVE the line i was reading. It breaks on Window Title too long and calls Insert this somewhere before running nuget. Something is making the PowerShell host window go nuts. |
|
Ok, so nuget is weird and does things to the Window Title WHEN called "vanilla". |
|
I'd go with door number two :) Just because it's decisive in telling the parser what to do,
|
|
Darn, bad news. As it turns out, this is not deterministic at all. It seems to fail about half the time in the git push scenario, without making any changes at all to the repo. I've seen it fail and pass with each if the suggested syntaxes, as well as the original one. So I'm not convinced that the different syntaxes we try actually really made any difference. These types of bugs suck :( |
|
When it breaks, does it always break on |
|
Cause I do not know, but be able to avoid the error if processing the output of nuget using the pipe. Maybe there is a problem with the processing of standard output. |
|
@shibayan How can i test it? I cloned this repo, ran build.cmd.. what do i do next? |
|
I figured out how to repro.. Yup, getting the same error. remote: Successfully installed 'WebGrease 1.5.2'.
remote: An error has occurred during web site deployment.
remote: Window title cannot be longer than 1023 characters.
remote: NuGet restore failed
remote: At D:\home\site\repository\deploy.ps1:92 char:3
remote: + nuget restore "$DEPLOYMENT_SOURCE\Dev14_Net46_Mvc5.sln"
remote: + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
remote: + CategoryInfo : InvalidArgument: (:) [], RuntimeException
remote: + FullyQualifiedErrorId : ArgumentLet me try a few things here. |
|
Looks like the problem is that each external binary (like nuget.exe is streaming the output to the Window Title). Don't ask me why, i can not understand this. However, wrapping all binary calls in
Here's what's changed in Start-Process "nuget.exe" -ArgumentList "restore $DEPLOYMENT_SOURCE\Dev14_Net46_Mvc5.sln" -Wait
...
Start-Process "$KUDU_SYNC_CMD" -ArgumentList "-v 50 -f $DEPLOYMENT_TEMP -t $DEPLOYMENT_TARGET -n $NEXT_MANIFEST_PATH -p $PREVIOUS_MANIFEST_PATH -i .git;.hg;.deployment;deploy.ps1" -Wait
...
Start-Process "$env:POST_DEPLOYMENT_ACTION" -WaitTry it out. Also the site is up at http://poshfix.azurewebsites.net EditWe got rid of the initial problem but now we have a regression. We can't grab the output of nuget.exe. |
|
|
@snobu Thank you for testing! |
|
I can repro this by doing: PS C:\Users\Adrian> $host.UI.RawUI.WindowTitle = $('A' * 1023)
PS C:\Users\Adrian> $host.UI.RawUI.WindowTitle = $('A' * 1024)
Exception setting "WindowTitle": "Window title cannot be longer than 1023 characters."
At line:1 char:1
+ $host.UI.RawUI.WindowTitle = $('A' * 1024)
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (:) [], SetValueInvocationException
+ FullyQualifiedErrorId : ExceptionWhenSettingAnd i am sure it's C:\Users\Adrian>cmd /c title aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
Not enough storage is available to process this command.Maybe it has something to do with the way Kudu invokes I'm still a bit worried cause we did fix it for now with the stdout redirect but it would still be beneficial to understand WHY it happens if we're going to make PowerShell the default deploy method. |
|
The following is the result of my examined. 1.Minimum of reproduction script (broken1.ps1) This script runs in the Kudu debug console. It is an error in the start-up of the process as described below. Often an error after a successful several times. 2.Show WindowTitle as follows. (broken2.ps1) You want to run this script. Interestingly, at first that contains the command line arguments to WindowTitle. Then the string is changed to random. After errors long string is displayed. 3.When the script to put the "| Out-Default" shall not be in error. However, WindowTitle is broken. (broken3.ps1) 4.Remove the execution of external process from script. (broken4.ps1) command line argument of powershell.exe have become WindowTitle. But default is a "Windows PowerShell". In my opinion there is a problem with PowerShell Host implementation of powershell.exe. As a result, not be initialized of WindowTitle. |
|
Can you retry the first test with Prepending start should launch a new host window. No idea if that changes anything. |
|
Now I try simple powershell host implementation. Will be report soon. |
|
Thanks @snobu and @takekazuomi for investigating this! I verified that the |
|
Result Of PowerShell Host Implementation Previous script (broken [1..4] .ps1) ran correctly. 1.broken1.ps1 2.broken2.ps1 3.broken3.ps1 4.broken4.ps1 also works. I was confirmed when deploy is successful by modifying the Dev14_Net46_Mvc5. Add YaPoshHost.exe (my powershell host) and modify .deployement After replacing a simple powershell host problem does not occur. |
|
Quality stuff there! I've also asked the man himself, Jeffrey Snover about this bug. He's always happy to help if PowerShell is in trouble :) Waiting for his reply. |
I tryed. same result. I tried to reproduce locally. The results we were able to reproduce.It is described here. I found CreateProces option in Kudu source then try to use same option. But not reproduce. https://github.com/takekazuomi/YaPoshHost/blob/master/PoshTest/ProcessTest.cs#L17 UPDATE |
|
Wonder if this makes any difference: Edit: Looks like that won't allow us to redirect stdout and stderr so it's a no go. |
|
@snobu and @takekazuomi @davidebbo This might be best as a workaround... |
|
@shibayan I tried that |
|
Ok, I pushed that change: 2d19772. It's Kudu script 1.0.1 on npm: https://www.npmjs.com/package/kuduscript. |
I added a support to generate a deployment script using PowerShell.