fix build in vscode #5453

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

build.psm1

+        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

build.psm1

@@ -569,10 +569,28 @@ Fix steps:
            $ReleaseVersion = $ReleaseTagToUse
        } else {
            $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

build.psm1

+            $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

build.psm1

-        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

.vscode/tasks.json

+            "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

.vscode/tasks.json

+        {
+            "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

.vscode/tasks.json

            "problemMatcher": "$msCompile"
        }
    ]
-}
+}


iSazonov

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

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

.vscode/tasks.json

+        {
+            "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

I think I'm caught up to your version of tasks.json but when I execute the Rebuild task (formerly Clean Build), I'm getting this error:

PowerShell output: c:\Users\Keith\GitHub\rkeithhill\PowerShell\debug
WARNING: Run Sync-PSTags to update tags
Unable to load file: "c:\Users\Keith\GitHub\rkeithhill\PowerShell\debug"
Execution of { & "~/.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" `
            --set-requested-execution-level "asInvoker" --set-version-string "LegalCopyright" "(C) Microsoft Corporation.  All Rights Reserved." } failed with exit code 1

Are you getting this error? FWIW I get this error from the command line so maybe my source is in a bad state.

UPDATE: Ah nevermind, I see you're making a tweak to that very part of build.psm1. I'll hang tight for the PR to be merged.

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 adityapatwardhan and TravisEz13 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix build in vscode #5453

fix build in vscode #5453

SteveL-MSFT commented Nov 14, 2017 •

edited

adityapatwardhan left a comment

daxian-dbw Nov 15, 2017

SteveL-MSFT Nov 15, 2017

daxian-dbw Nov 15, 2017

SteveL-MSFT Nov 15, 2017

daxian-dbw Nov 15, 2017

SteveL-MSFT Nov 15, 2017

daxian-dbw Nov 15, 2017

SteveL-MSFT Nov 15, 2017

rkeithhill commented Nov 15, 2017 •

edited

SteveL-MSFT commented Nov 15, 2017

rkeithhill Nov 15, 2017

rkeithhill Nov 15, 2017

SteveL-MSFT Nov 15, 2017

SteveL-MSFT commented Nov 15, 2017

iSazonov Nov 15, 2017

SteveL-MSFT Nov 15, 2017

iSazonov left a comment

rkeithhill Nov 15, 2017

rkeithhill Nov 15, 2017 •

edited

SteveL-MSFT Nov 15, 2017

rkeithhill Nov 15, 2017 •

edited

SteveL-MSFT Nov 15, 2017

rkeithhill commented Nov 15, 2017 •

edited

fix build in vscode #5453

fix build in vscode #5453

Conversation

SteveL-MSFT commented Nov 14, 2017 • edited

adityapatwardhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill commented Nov 15, 2017 • edited

SteveL-MSFT commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveL-MSFT commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill Nov 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill Nov 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill commented Nov 15, 2017 • edited

SteveL-MSFT commented Nov 14, 2017 •

edited

rkeithhill commented Nov 15, 2017 •

edited

rkeithhill Nov 15, 2017 •

edited

rkeithhill Nov 15, 2017 •

edited

rkeithhill commented Nov 15, 2017 •

edited