Fix Start-PSBuild on WSL if repository was already built on Windows #5346

bergmeister · 2017-11-05T18:29:22Z

To fix #5194: Use the first 3 characters of the passed -Runtime parameter for the .inc file (the full runtime name could not be used since the separation characters ike e.g. the the hyphen would make dotnet crash when building).

A separate issue is that the runtime does not get determined if is not being passed (which is OK since dotnet takes then care of choosing the right one) but maybe given this scenario would it make sense to determine and set it as a convenience?

Note that the '-Runtime' parameter needs to be passed in this case.

rkeithhill · 2017-11-05T20:09:53Z

This should slot in nicely with my tweak to build.psm1 #5341

rkeithhill · 2017-11-05T21:58:08Z

I think you need to update this .gitignore file to make it handle the platform specific inc files:

40:121ms> gc .\src\TypeCatalogGen\.gitignore
powershell.inc

I was noticing that Git thought I had a new file with powershell_win.inc.

… because the file can now optionally end in the runtime prefix.

bergmeister · 2017-11-05T22:21:25Z

Thanks. Well spotted, I updated it to ignore now powershell**.inc. I find it a bit odd that there is a special .gitignore file in this folder and am wondering if we can move this to the top level .gitignore file?
I see that @andschwa moved out ignoring the .inc file from the top level .gitignore file in this commit. What was the reasoning for this?

andyleejordan · 2017-11-05T22:35:21Z

@bergmeister Because this ignore rule doesn't apply to anywhere else in the repository, just keeping it simple 😄

bergmeister · 2017-11-05T22:53:18Z

@andschwa My thinking is rather that if there will never be any other .inc files somewhere else, it would be nice to put it into the top one to have a more central place, especially since there is only one entry in it. But this is probably more of a stylistic preference with no 'right' answer depending on how big the central place can become (to me the output of gci -Recurse .gitignore | % {cat $_} did not seem to be too long)... But I'll leave it then as it is if that's your preference.

TravisEz13 · 2017-11-06T19:44:14Z

build.psm1

-    if ($TypeGen -or -not (Test-Path "$PSScriptRoot/src/System.Management.Automation/CoreCLR/CorePsTypeCatalog.cs")) {
+    $incFileName = 'powershell'
+    if (-not [string]::IsNullOrEmpty($Runtime)) {
+        $incFileName = "$($incFileName)_$($Runtime.Substring(0,3))" # use the first 3 characters only because otherwise dotnet will crash due to the separation characters


I believe our guidance is to put the comment on a separate line.

you could replace the seperation character.... not sure it's needed though.

You're right about the comment. I noticed that only after the commit but I will fix that. The separation character is only for readability because to me powershell_win or powerhell_lin is easier and faster to read than powershellwin or powershelllin

TravisEz13

Looks good with minor comments.

@daxian-dbw and/or @iSazonov should review as well.

…tails

bergmeister · 2017-11-06T20:58:31Z

NB: The build was red because NuGet has a global outage (see here)

iSazonov · 2017-11-07T04:30:09Z

build.psm1

+        # Append the first 3 characters fo the Runtime only because otherwise dotnet will crash due to the separation characters. The underscore is only for better readability.
+        $incFileName = "$($incFileName)_$($Runtime.Substring(0,3))"
+    }
+    if ($TypeGen -or -not (Test-Path "$PSScriptRoot/TypeCatalogGen/$($incFileName).inc")) {


We use $incFileName).inc so many times so make sense add:

$incFileName = ($incFileName).inc

Ok, I have addressed that.

iSazonov · 2017-11-07T04:37:50Z

I'd prefer to fix the issue more natively like #3870.

Also I'm not sure that's the only runtime addiction. Should we clean up if our files contains "#if UNIX"?

…ication as suggested in PR. Tested again that it still works when building on Windows first using 'Start-PSBuild' and then 'Start-PSBuild -Runtime linux-x64' on WSL (ubuntu16)

daxian-dbw · 2017-11-07T21:54:22Z

build.psm1

    # handle TypeGen
-    if ($TypeGen -or -not (Test-Path "$PSScriptRoot/src/System.Management.Automation/CoreCLR/CorePsTypeCatalog.cs")) {
+    $incFileName = 'powershell.inc'
+    if (-not [string]::IsNullOrEmpty($Runtime)) {


Can you use $Options.Runtime? When -Runtime is not specified, we will figure out the current RID via dotnet --info, which will be captured by $Options.Runtime

The $Options.Runtime will always be 'NotNullOrEmpty', so maybe you can have:

$incFileName = "powershell_$($Runtime.Substring(0,3)).inc"

Good idea. Yes, we can use $Options.Runtime indeed instead. I was not aware that you already figure it out automatically when it is not being provided. Shall I maybe add this information as a comment to the $Runtime parameter?

You mean the -Runtime parameter of Start-PSBuild? Sure you can add a comment there.

daxian-dbw · 2017-11-07T21:59:47Z

build.psm1

+    $incFileName = 'powershell.inc'
+    if (-not [string]::IsNullOrEmpty($Runtime)) {
+        # File name must be different for Windows and Linux to allow build on Windows and WSL.
+        # Append the first 3 characters fo the Runtime only because otherwise dotnet will crash due to the separation characters. The underscore is only for better readability.


characters fo the Runtime

A typo.

otherwise dotnet will crash due to the separation characters

Could you please share more information on this?

Thanks for spotting the typo and thanks for coming back about the crash. When I started out I was experimenting with different approaches (one of was to put the inc file into a runtime specific folder) and I actually cannot repro the crash anymore and will therefore use the full $Runtime string instead of the first 3 characters.

daxian-dbw · 2017-11-07T22:00:31Z

build.psm1

+    if (-not [string]::IsNullOrEmpty($Runtime)) {
+        # File name must be different for Windows and Linux to allow build on Windows and WSL.
+        # Append the first 3 characters fo the Runtime only because otherwise dotnet will crash due to the separation characters. The underscore is only for better readability.
+        $incFileName = "$($incFileName)_$($Runtime.Substring(0,3)).inc"


Maybe just use powershell_$($Runtime.Substring(0,3)).inc? Otherwise it would beome powershell.inc_xxx.inc

This change has now been undone since we do not need that logic any more due to the usage of $Options.Runtime

daxian-dbw · 2017-11-07T22:03:02Z

build.psm1

+        # Append the first 3 characters fo the Runtime only because otherwise dotnet will crash due to the separation characters. The underscore is only for better readability.
+        $incFileName = "$($incFileName)_$($Runtime.Substring(0,3)).inc"
+    }
+    if ($TypeGen -or -not (Test-Path "$PSScriptRoot/TypeCatalogGen/$($incFileName)")) {


$($incFileName) can be replaced by $incFileName

No, this is one of the gotchas about PowerShell string interpolation because PowerShell allows underscores for variable names. Therefore PowerShell would then look for a variable named incFileName_ instead of incFileName. As a general rule, I always wrap variables in a string inside $() because it also safeguards against cases like calling a property on an object...

In the case of "$($incFileName)_xxx.inc", you can replace it with "${incFileName}_xxx.inc". PowerShell support the varialbe syntax ${var name}

daxian-dbw · 2017-11-07T22:35:23Z

build.psm1

+    (
+        [Parameter(Mandatory=$true)]
+        [ValidateNotNullOrEmpty()]
+        $IncFileName


Maybe we should have 'powershell.inc' as the default value for this parameter, so that Start-TypeGen without parameter will continue to work.

I can do that if people actually call Start-TypeGen? I always use the -ResGen swich on Start-PSBuild because the reason why I need update ResGen is because I need to build anyway. Instead of having the default, why not call New-PSOptions and grab the determined Runtime, which is what Start-PSBuild does as well.

It's not commonly needed, but when debugging anything about the type catalog, it's handy to just run Start-TypeGen without building the rest of the projects. For that purpose, a simple default value like powershell.inc would be sufficient, no need to call New-Options to figure out the runtime.

And a minor comment: the variable name you used in the rest of this function is incFileName, maybe it can be changed to IncFileName.

daxian-dbw · 2017-11-07T22:38:56Z

build.psm1

    Push-Location "$PSScriptRoot/src/Microsoft.PowerShell.SDK"
    try {
-        $ps_inc_file = "$PSScriptRoot/src/TypeCatalogGen/powershell.inc"
+        $ps_inc_file = "$PSScriptRoot/src/TypeCatalogGen/$($incFileName)"


$($incFileName) can be replaced by $incFileName

In this case we can do that but what do you think about my comment above where this is not always possible?

PowerShell support this syntax for variable: ${var name}, so you can use "${varname}_xxx.inc" in that case.

bergmeister · 2017-11-09T12:45:05Z

I'll address the comments next Monday when I'm back from travelling.

… to be necessary.

…rmined if the $Runtime is not being provided.

bergmeister · 2017-11-13T20:54:37Z

@daxian-dbw Thanks for your useful feedback. I have addressed all issues and left comments there. But basically the 2 remaining questions are:

Should we be strict about string interpolation (i.e. use $()) since there are gotchas as I pointed out?
Should Start-TypeGen have a default value for the new $IncFileName parameter or should we automatically determine it in there similarly as in Start-PSBuild?

daxian-dbw · 2017-11-14T22:49:54Z

build.psm1

+    # Handle TypeGen
+    # .inc file name must be different for Windows and Linux to allow build on Windows and WSL.
+    $incFileName = "powershell_$($Options.Runtime).inc"
+    if ($TypeGen -or -not (Test-Path "$PSScriptRoot/TypeCatalogGen/$($incFileName)")) {


$($incFileName)

Subexpression is not needed here, you can just use $incFileName

daxian-dbw · 2017-11-14T22:51:47Z

build.psm1

+        # Append the first 3 characters fo the Runtime only because otherwise dotnet will crash due to the separation characters. The underscore is only for better readability.
+        $incFileName = "$($incFileName)_$($Runtime.Substring(0,3)).inc"
+    }
+    if ($TypeGen -or -not (Test-Path "$PSScriptRoot/TypeCatalogGen/$($incFileName)")) {


In the case of "$($incFileName)_xxx.inc", you can replace it with "${incFileName}_xxx.inc". PowerShell support the varialbe syntax ${var name}

daxian-dbw · 2017-11-14T22:53:28Z

build.psm1

+    (
+        [Parameter(Mandatory=$true)]
+        [ValidateNotNullOrEmpty()]
+        $IncFileName


It's not commonly needed, but when debugging anything about the type catalog, it's handy to just run Start-TypeGen without building the rest of the projects. For that purpose, a simple default value like powershell.inc would be sufficient, no need to call New-Options to figure out the runtime.

daxian-dbw · 2017-11-14T22:54:30Z

build.psm1

    Push-Location "$PSScriptRoot/src/Microsoft.PowerShell.SDK"
    try {
-        $ps_inc_file = "$PSScriptRoot/src/TypeCatalogGen/powershell.inc"
+        $ps_inc_file = "$PSScriptRoot/src/TypeCatalogGen/$($incFileName)"


PowerShell support this syntax for variable: ${var name}, so you can use "${varname}_xxx.inc" in that case.

daxian-dbw · 2017-11-14T22:59:32Z

@bergmeister For the two questions, I answered them in the corresponding comment threads: #5346 (comment) and #5346 (comment)

Use default value for Start-TypeGen for convenience and minor syntax changes. Tested again that it still works when building on Windows first and then on WSL

bergmeister · 2017-11-14T23:45:55Z

@daxian-dbw Thanks for the review. I have addressed your comments.

daxian-dbw

LGTM. Thanks @bergmeister!

Fix build on WSL if repository was already built on Windows.

e4fa4e8

Note that the '-Runtime' parameter needs to be passed in this case.

bergmeister requested review from TravisEz13, adityapatwardhan and daxian-dbw as code owners November 5, 2017 18:29

Update .gitignore to match powershell**.inc instead of powershell.inc…

8d4a8af

… because the file can now optionally end in the runtime prefix.

TravisEz13 reviewed Nov 6, 2017

View reviewed changes

TravisEz13 approved these changes Nov 6, 2017

View reviewed changes

Put comment on single line as per style guide and add more comment de…

af094e3

…tails

iSazonov reviewed Nov 7, 2017

View reviewed changes

Include file extension in $incFileName variable to reduce '.inc' dupl…

e1fde8f

…ication as suggested in PR. Tested again that it still works when building on Windows first using 'Start-PSBuild' and then 'Start-PSBuild -Runtime linux-x64' on WSL (ubuntu16)

daxian-dbw requested changes Nov 7, 2017

View reviewed changes

bergmeister added 2 commits November 13, 2017 20:16

Fix typo and do not trim the $Runtime string since this does not seem…

adcf857

… to be necessary.

Use $Options.Runtime instead of $Runtime because the former gets dete…

a78e0c0

…rmined if the $Runtime is not being provided.

daxian-dbw reviewed Nov 14, 2017

View reviewed changes

daxian-dbw requested changes Nov 14, 2017

View reviewed changes

Addressed minor suggestions made in PR 5346.

4625d83

Use default value for Start-TypeGen for convenience and minor syntax changes. Tested again that it still works when building on Windows first and then on WSL

daxian-dbw approved these changes Nov 15, 2017

View reviewed changes

daxian-dbw self-assigned this Nov 15, 2017

TravisEz13 approved these changes Nov 15, 2017

View reviewed changes

daxian-dbw merged commit 019cfee into PowerShell:master Nov 15, 2017

Fix Start-PSBuild on WSL if repository was already built on Windows #5346

Fix Start-PSBuild on WSL if repository was already built on Windows #5346

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rkeithhill commented Nov 5, 2017

Uh oh!

rkeithhill commented Nov 5, 2017

Uh oh!

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

Uh oh!

Uh oh!

andyleejordan commented Nov 5, 2017

Uh oh!

bergmeister commented Nov 5, 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!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Nov 7, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister Nov 13, 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!

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!

daxian-dbw Nov 14, 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!

bergmeister Nov 13, 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!

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!

bergmeister commented Nov 9, 2017

Uh oh!

bergmeister commented Nov 5, 2017 •

edited

Loading

bergmeister commented Nov 5, 2017 •

edited

Loading

bergmeister commented Nov 6, 2017 •

edited

Loading

bergmeister Nov 13, 2017 •

edited

Loading

daxian-dbw Nov 14, 2017 •

edited

Loading

bergmeister Nov 13, 2017 •

edited

Loading

daxian-dbw Nov 14, 2017 •

edited

Loading