iframe-proxy

SteveL-MSFT · 2017-11-14T23:47:43Z

There were several issues:

In vscode, depending on where you started it from, it may not get the git commit id so it ends up empty. Add a default value to use.

From the console, the commit id was typically in a form that is not parse-able as a fileversion. Fix is to chop off everything right of the dash.

It also seems that building from the console and building from vscode, the output path may or may not include pwsh, however, rcedit needs the actual executable in the path.

Fix #5440

adityapatwardhan

LGTM

daxian-dbw · 2017-11-15T01:18:11Z

+        Write-Verbose "Release version: $ReleaseVersion" -Verbose
+
+        $pwshPath = $Options.Output
+        if ($Environment.IsWindows) {


The whole RCEdit work is in if ($Environment.IsWindows) { block (see line 561), so no need to split the code path here.

Yeah, forgot rcedit only applied to Windows, will fix

daxian-dbw · 2017-11-15T01:20:07Z

            $ReleaseVersion = (Get-PSCommitId) -replace '^v'
+            $fileVersion = $ReleaseVersion.Split("-")[0]
+        }
+        if (!$ReleaseVersion) {


Can you please add a comment about "In vscode, depending on where you started it from, it may not get the git commit id so it ends up empty. Add a default value to use."?

daxian-dbw · 2017-11-15T01:25:59Z

+            $fileVersion = "6.0.0"
+        }
+
+        Write-Verbose "Release version: $ReleaseVersion" -Verbose


Please use log like rest of the file. I think it's better to make the message more specific on the RCEdit operation, like "Embed the icon resource to pwsh.exe, use release version: $ReleaseVersion"

I'll just remove this, only needed it to figure out why it wasn't working

daxian-dbw · 2017-11-15T01:26:30Z

-        Start-NativeExecution { & "~/.rcedit/rcedit-x64.exe" "$($Options.Output)" --set-icon "$PSScriptRoot\assets\Powershell_black.ico" `
-            --set-file-version $ReleaseVersion --set-product-version $ReleaseVersion --set-version-string "ProductName" "PowerShell Core 6" `
+        Start-NativeExecution { & "~/.rcedit/rcedit-x64.exe" $pwshPath --set-icon "$PSScriptRoot\assets\Powershell_black.ico" `
+            --set-file-version $fileVersion --set-product-version $ReleaseVersion --set-version-string "ProductName" "PowerShell Core 6" `


$fileVersion is not set when $ReleaseTagToUse exists.

rkeithhill · 2017-11-15T03:17:33Z

You might want to try this tasks.json:

{
    "version": "2.0.0",

    "windows": {
        "options": {
            "shell": {
                "executable": "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe",
                "args": [
                    "-NoProfile",
                    "-ExecutionPolicy",
                    "Bypass",
                    "-Command"
                ]
            }
        }
    },
    "linux": {
        "options": {
            "shell": {
                "executable": "/usr/bin/pwsh",
                "args": [
                    "-NoProfile",
                    "-Command"
                ]
            }
        }
    },
    "osx": {
        "options": {
            "shell": {
                "executable": "/usr/local/bin/pwsh",
                "args": [
                    "-NoProfile",
                    "-Command"
                ]
            }
        }
    },

    "tasks": [
        {
            "label": "Bootstrap",
            "type": "shell",
            "command": "Import-Module ${workspaceFolder}/build.psm1; Start-PSBootstrap",
            "problemMatcher": []
        },
        {
            "label": "Clean Build",
            "type": "shell",
            "command": "Import-Module ${workspaceFolder}/build.psm1; Start-PSBuild -Clean",
            "problemMatcher": []
        },
        {
            "label": "Build",
            "type": "shell",
            "command": "Import-Module ${workspaceFolder}/build.psm1; Start-PSBuild -Output ${workspaceFolder}/debug",
            "group": {
                "kind": "build",
                "isDefault": true
            },
            "problemMatcher": "$msCompile"
        },
        {
            "label": "Test",
            "type": "shell",
            "command": "Import-Module ${workspaceFolder}/build.psm1; Start-PSPester -Path ${workspaceFolder}/test -Tag CI",
            "group": {
                "kind": "test",
                "isDefault": true
            },
            "problemMatcher": "$pester"
        }
    ]
}

There seems to be an issue with the $msCompile problem matcher at the moment but that's an issue for VSCode. BTW why does the build task specify -Output ./debug? Doesn't it put the output in a reasonable location by default? Also, any reason not to use Windows PowerShell on Windows? If so, we can always set the executable to pwsh.exe on Windows.

Also, I wonder if we should specify a tag for testing within VSCode - perhaps CI? I think using CI would get rid of the failures I see due to not running VSCode as admin.

I can submit this as a PR if you'd like or if you just want to take it and run - that's fine too.

SteveL-MSFT · 2017-11-15T03:43:13Z

I'll add a slightly modified version of @rkeithhill 's tasks.json. Thanks for that!

added v2.0.0 tasks.json thanks to Keith Hill

rkeithhill · 2017-11-15T03:53:48Z

+            "label": "Clean Build",
+            "type": "shell",
+            "command": "Import-Module ${workspaceFolder}/build.psm1; Start-PSBuild -Clean",
+            "problemMatcher": []


Doh! I forgot to put the $msCompile problem matcher here. I could be wrong about this but Start-PSBuild -Clean doesn't just clean. It cleans first and then builds, right?

rkeithhill · 2017-11-15T03:55:26Z

+        {
+            "label": "Test",
+            "type": "shell",
+            "command": "Import-Module ${workspaceFolder}/build.psm1; Start-PSPester -Path ${workspaceFolder}/test",


You should consider adding the -Tag CI parameter here. Running VSCode as admin is not something most folks do and if you run these tests not elevated, you get a lot of failures. You could also add two tasks - one that does a full test run and another that tests just the CI tests.

Actually, I think I'll remove this task as intent is people run CI + Feature (which is default), but you are right that the tests expect user to be elevated and, in general, shouldn't be running vscode elevated.

SteveL-MSFT · 2017-11-15T04:04:18Z

@rkeithhill to answer some of your questions:

the debug folder is used as it's included in .gitignore
since this tasks.json is only used to build pwsh, I think it's preferable to depend on pwsh rather than Windows PowerShell (the rest of the tooling expects it)
-Clean cleans then builds

iSazonov · 2017-11-15T04:47:29Z

        }
    ]
-}
+}


iSazonov

@SteveL-MSFT @rkeithhill Thanks for the great PR!

rkeithhill · 2017-11-15T04:57:47Z

+        {
+            "label": "Build",
+            "type": "shell",
+            "command": "Import-Module ${workspaceFolder}/build.psm1; Start-PSBuild -Output (Join-Path ${workspaceFolder} debug)",


Nice! Can you add the same parameter to the Clean Build task above? Also, Clean Build reads a little awkward. What if we called this task Rebuild? On VS at least, that implies Clean, then Build.

BTW I was just going to warn you about this issue when I saw you not only already knew about it but solved it. 👍 Also, I tagged onto this VSCode issue on the $mscompile problem matcher not working - microsoft/vscode#34660

Will add to Clean Build which I think is named fine

@SteveL-MSFT I got a response from the VSCode team on the $mscompile issue. All we need to do is add the command line arg - /property:GenerateFullPaths=true. I tried putting this around line 485:

$Arguments += "/property:GenerateFullPaths=true"

And it works! There may be other places where this arg needs to be provided.

Will add. Thanks!

rkeithhill · 2017-11-15T05:12:26Z

fix build in vscode

6b93dee

SteveL-MSFT assigned daxian-dbw Nov 14, 2017

SteveL-MSFT requested a review from daxian-dbw November 14, 2017 23:47

SteveL-MSFT requested review from TravisEz13 and adityapatwardhan as code owners November 14, 2017 23:47

adityapatwardhan approved these changes Nov 15, 2017

View reviewed changes

daxian-dbw requested changes Nov 15, 2017

View reviewed changes

address PR feedback

c4d0993

added v2.0.0 tasks.json thanks to Keith Hill

rkeithhill reviewed Nov 15, 2017

View reviewed changes

removed Test task as running tests require elevation on Windows

c1f0bea

SteveL-MSFT force-pushed the vscode-build branch from 15b6e48 to c1f0bea Compare November 15, 2017 04:01

iSazonov reviewed Nov 15, 2017

View reviewed changes

iSazonov approved these changes Nov 15, 2017

View reviewed changes

rkeithhill reviewed Nov 15, 2017

View reviewed changes

SteveL-MSFT added 2 commits November 14, 2017 21:42

address PR feedback

b558516

add /property:GenerateFullPaths=true to build

83beea4

daxian-dbw approved these changes Nov 15, 2017

View reviewed changes

daxian-dbw merged commit 2f8e691 into PowerShell:master Nov 15, 2017

SteveL-MSFT deleted the vscode-build branch November 15, 2017 20:09

SteveL-MSFT restored the vscode-build branch November 21, 2017 18:49

SteveL-MSFT deleted the vscode-build branch October 26, 2018 21:34

                       }
                   ]
-              }
+              }

                
                    No newline at end of file

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

SteveL-MSFT commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adityapatwardhan left a comment

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!

rkeithhill commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Nov 15, 2017

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!

SteveL-MSFT commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkeithhill Nov 15, 2017 • 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!

rkeithhill Nov 15, 2017 • 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!

rkeithhill commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SteveL-MSFT commented Nov 14, 2017 •

edited

Loading

rkeithhill commented Nov 15, 2017 •

edited

Loading

rkeithhill Nov 15, 2017 •

edited

Loading

rkeithhill Nov 15, 2017 •

edited

Loading

rkeithhill commented Nov 15, 2017 •

edited

Loading