Change PSES to be buildable as a standalone #647

rjmholt · 2018-03-29T22:24:35Z

When PSES builds, it should be responsible for its own modules. So I've added a build rule that populates its /module/ directory. This moves the modules.json file over from the VSCode extension as well.

So with this, PSES should be generally self contained and ready to run after building.

TylerLeonhardt

Just a question on some duplicate code I see. Otherwise LGTM!

TylerLeonhardt · 2018-03-30T17:48:10Z

PowerShellEditorServices.build.ps1


 if ($PSVersionTable.PSEdition -ne "Core") {
    Add-Type -Assembly System.IO.Compression.FileSystem
 }

+function Import-SubmoduleConfigurations


Is this function needed?

I would think that in RestorePsesModules you could just do:

$modules = Get-Content -Raw "$PSScriptRoot/modules.json" | ConvertFrom-Json $modules.PSObject.Properties | ForEach-Object { $params = @{ Name = $_.Name MinimumVersion = $_.Value.MinimumVersion MaximumVersion = $_.Value.MaximumVersion Path = "$PSScriptRoot/modules/" } if ($script:SaveModuleSupportsAllowPrerelease) { $params += @{ AllowPrerelease = $moduleInstallDetails.AllowPrerelease } } Save-Module @params }

TylerLeonhardt · 2018-03-31T01:16:56Z

AppVeyor so drunk

rjmholt · 2018-03-31T01:42:13Z

No, those tests were failing on my machine too. I need to work out why that is.

rjmholt · 2018-04-04T23:36:15Z

Ok, I've now run this against vscode-powershell #1252 and verified that it works.

TylerLeonhardt

Just a couple comments!

TylerLeonhardt · 2018-04-05T00:17:46Z

PowerShellEditorServices.build.ps1

+    }
+
+    # Save each module in the modules.json file
+    foreach ($moduleName in $moduleInfos.Keys)


We have two foreach's one right after the other. Any chance we can combine those?

The only problem is that if there's a bad field in the json file, we'll install half the things and then abort.

Ideally, we could have an explicit failure line after the first foreach, but I'm not sure what the best way to detect a failure is or what the best way to fail in a task is.

eh. Not worth implementing. What you have is good :)

TylerLeonhardt · 2018-04-05T00:21:38Z

test/PowerShellEditorServices.Test.Host/ServerTestsBase.cs

+            /*
+            System.Console.WriteLine("Test Process PID: " + Process.GetCurrentProcess().Id);
+            System.Console.WriteLine("Service Process PID: " + this.serviceProcess.Id);
+            System.Console.ReadKey();


Probably should delete this commented out code

Ideally I'd like to wrap it in a conditional, unless you think that will make the code too ugly. I can just imagine wanting to use this again.

TylerLeonhardt · 2018-04-05T00:27:38Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

-                var sessionState = InitialSessionState.CreateDefault2();
-                sessionState.ImportPSModulesFromPath(scriptAnalyzerModuleInfo.ModuleBase);
+                InitialSessionState sessionState = InitialSessionState.CreateDefault2();
+                sessionState.ImportPSModulesFromPath(GetModulePathFromModuleInfo(scriptAnalyzerModuleInfo));


Since we're just grabbing PSSA from the module path, I wonder if there's a helper variable somewhere that already has the Module path in it. Then we don't need to do this fancy path logic with PSSA just to get the module path.

Yeah that's a good idea. It may even be a matter of querying the variable.

I've been thinking about this. It looks like the way to access this directly is kept internal. It's present as a property of ExecutionContext, but getting that out of the current runspace would be tricky and possibly error prone.

So another option is to get our powershell session to evaluate $env:PSModulePath, but I don't think that's very performant. My bigger concern is that, even if we don't consider the possibility that the module path could be changed in some way we don't expect, we still would be trying to import every module in the PSES module directory. Which we don't need in the analysis part and could potentially be quite slow.

But it's also true that I've overengineered the module path resolution bit already. Since we sort of know specifically whether PSSA will be using a versioned path or not. Except that this broke before. And ideally we could eventually move this method out and make it a static utility function.

Keep in mind PSSA can come from anywhere, we definitely don't want to lock it to the bundled version.

Ah, that's a very good point (which admittedly I had forgotten)!

But I'm pretty sure the new code still does that. It still retrieves the module given by Get-Module PSScriptAnalyzer and gets the path. The problem is that PSModuleInfo doesn't have a good way of representing its base path; PSModuleInfo.ModuleBase gives you what could be a versioned path that doesn't work with Import-Module (or the C# API equivalent used here).

So the only bit I added was to prune the versioned directory from the path if it's the leaf directory.

Oh for sure, I should have clarified that I was commenting on the thread and not the actual changes. Side note, I usually just target the psd1 in these situations. Something like this

Path.ChangeExtension( Path.Combine( moduleInfo.ModuleBase, moduleInfo.Name), ".psd1"))

But your approach accounts for modules without a manifest so it's more flexible in general.

Well actually I wanted to do that (or use whatever PSModuleInfo.Path gave me, which is either a psd1 or a psm1). Or there's also an overload of InitialSessionState.ImportPSModule that takes an IEnumerable<ModuleSpecification>, and there's a constructor for ModuleSpecification that takes a PSModuleInfo.

But, it turns out that ImportPSModulesFromPath calls ModuleUtils.GetDefaultAvailableModuleFiles internally, which only accepts paths to directories as arguments.

(And the second approach doesn't seem to populate the ModuleSpecification fields other than Name.)

I just wish PowerShell exposed a more convenient way to manipulate its modules and module loading.

Ah right, I always forget the differences between those methods. I typically use InitialSessionState.ImportPSModule, which does work with psd1 files.

I just wish PowerShell exposed a more convenient way to manipulate its modules and module loading.

👍 👍 👍

In a quest to work out what might be best, I noticed that InitialSessionState.ImportPSModulesFromPath() in turn just converts the modules it finds into ModuleSpecifications and feeds them to InitialSessionState.ImportPSModule.

Anyway, I went to look at why this particular command can take a ModuleSpecification with everything but the Name field set to null, and have the Name be just a name or a path to a specification, or a whole heap of other things, and just work.

Well, turns out no matter what you do, PowerShell itself just calls Import-Module (see here).

~~So I might actually just change our code to do that directly (even though I don't really like the idea of that)...~~

The method on the sessionState variable conveniently handles the complexity of runspaces and sessions and making sure we get a good PowerShell instance to import the module into. So I'm going to use the path.

rjmholt · 2018-04-05T01:56:04Z

PowerShellEditorServices.build.ps1

@@ -179,7 +186,7 @@ task TestProtocol -If { !$script:IsUnix} {
 task TestHost -If { !$script:IsUnix} {
    Set-Location .\test\PowerShellEditorServices.Test.Host\
    exec { & $script:dotnetExe build -c $Configuration -f net452 }
-    exec { & $script:dotnetExe xunit -configuration $Configuration -framework net452 -verbose -nobuild -x86 }


Also, I have no idea what the consequences of this change could be. Because I'm not sure why that was there originally.

I suspect it's because VSCode use to only be 32bit but now it has a 64bit version so we can probably safely remove this.

TylerLeonhardt

Only the one comment regarding our conversation.

TylerLeonhardt · 2018-04-10T20:16:33Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

-                sessionState.ImportPSModulesFromPath(scriptAnalyzerModuleInfo.ModuleBase);
+                this._scriptAnalyzerModuleInfo = FindPSScriptAnalyzerModule(logger);
+                InitialSessionState sessionState = InitialSessionState.CreateDefault2();
+                sessionState.ImportPSModule(new [] { this._scriptAnalyzerModuleInfo.Path });


We talked about this offline but I think it would be easier just to put "PSScriptAnalyzer" here instead of the path received from calling the FindPSScriptAnalyzerModule(logger); function. This also means that the FindPSScriptAnalyzerModule function can be totally removed from the code which is also a huge plus in my opinion.

I'd say give it a shot, if it doesn't work then this is fine.

rjmholt · 2018-04-18T16:14:24Z

Just got rid of the "always get the latest PSScriptAnalyzer" logic. Now it works on the module path alone (and we get a startup performance boost as well I suppose). Now, if you put a different version (higher or lower) of PSSA on your module path, that will load before ours. Which I think makes more sense than trying to second guess the module path concept.

TylerLeonhardt · 2018-04-18T16:18:26Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

-                return null;
-            }
-        }
-


deleting an entire function 😍 this is awesome

We should probably verify that it actually works though. One sec.

Maybe I'm missing something but the deleted function seems to be replicating the Import-Module logic for loading a module. That is, ipmo always loads the latest version in the PSModulePath anyway. Unless the caller specifically needs the PSModuleInfo for some reason other than Import-Module.

@rkeithhill Actually, ipmo does not load the latest version, it loads the first version it finds. This could be a bug in PowerShell Core, but here's what I did:

PS > $env:PSModulePath /Users/tylerleonhardt/.local/share/powershell/Modules:/usr/local/share/powershell/Modules:/usr/local/microsoft/powershell/6.0.2/Modules ## ORDER: # 1: /Users/tylerleonhardt/.local/share/powershell/Modules # 2: /usr/local/share/powershell/Modules # 3: /usr/local/microsoft/powershell/6.0.2/Modules # What's in #1's Pester PS > gci /Users/tylerleonhardt/.local/share/powershell/Modules/Pester Directory: /Users/tylerleonhardt/.local/share/powershell/Modules/Pester Mode LastWriteTime Length Name ---- ------------- ------ ---- d----- 2/9/18 4:14 PM 4.1.1 # What's in #2's Pester PS > gci /usr/local/share/powershell/Modules/Pester/ Directory: /usr/local/share/powershell/Modules/Pester Mode LastWriteTime Length Name ---- ------------- ------ ---- d----- 4/13/18 12:20 PM 4.3.1 # Load Pester - see it's 4.1.1 PS > ipmo Pester WARNING: OS Information retrieval is not possible, reports will contain only partial system data /Users/tylerleonhardt/Desktop/Tech/PowerShell/vscode/PowerShellEditorServices [master ≡]> Get-Module Pester ModuleType Version Name ExportedCommands ---------- ------- ---- ---------------- Script 4.1.1 Pester {Add-AssertionOperator, AfterAll, AfterEach, AfterEachFeature...}

It turns out that deep down, InitialSessionState.ImportPSModule() actually spins up a PowerShell instance and calls ipmo anyway. So ideally we don't duplicate the way it works, we just use its logic. I think this should work now with the change I just made. It looks like Import-Module searches sequentially along the path until it finds a module with the right name and then returns that -- code is here.

Well that is somewhat disappointing behavior. But it would explain why we had that method. Another option would be to import with -MinimumVersion but that might be a pain to keep current. Anytime we used a new PSSA feature we'd have to update the MinimumVersion parameter.

Agreed - AFAIK that's how "PATH" works too.

Another option would be to import with -MinimumVersion but that might be a pain to keep current

I like this idea. I don't see crazy PSSA changes just for PSES coming our way in the near future and I don't think they'll happen often. It's a small change to make us that much more resilient against someone who still has PSSA 1.0 or something that doesn't work with PSES.

@rjmholt sorry we keep giving you more work!

I just remembered we're not actually doing Import-Module it's the sessionState.ImportPSModule and there doesn't really seem to be a good way to specify MinimumVersion in its overloads so I've fine with doing what we have in this PR.

rjmholt · 2018-04-18T16:39:51Z

I've also noticed that there's a few lines in the Start-EditorServices.ps1 script that want to install the right version of EditorServices. But I'm thinking we should remove those...

TylerLeonhardt · 2018-04-18T16:49:30Z

looks like a zip was added:

https://github.com/PowerShell/PowerShellEditorServices/pull/647/files#diff-837b6c39bb63ded0123f1da825124299

can you add something to the gitignore so that we don't have this anymore? I'm guessing it's the "artifact" from doing a full Invoke-Build

TylerLeonhardt · 2018-04-18T16:57:17Z

@rjmholt

I've also noticed that there's a few lines in the Start-EditorServices.ps1 script that want to install the right version of EditorServices. But I'm thinking we should remove those...

are you talking about:
https://github.com/PowerShell/PowerShellEditorServices/blob/master/module/Start-EditorServices.ps1#L250-L261

We can delete that since that module doesn't actually exist. I think David wanted some way to bring in PowerShell Editor Services if it didn't exist but due to some folks being in environments without internet, bundling everything together was the only option.

rjmholt · 2018-04-18T17:10:59Z

Ok, needed to put a PowerShell call back in to verify that we actually have PSScriptAnalyzer available and set all the _hasScriptAnalyzer checks to true (although I don't think they're necessary because the constructor fails if it isn't....).

Anyway, this class I'm touching is ripe for some refactoring (for starters because I don't think we should be calling PowerShell in the constructor at all, and should make a static factory method).

rjmholt · 2018-04-18T18:43:19Z

So my current question is:

The intent of this PR is to make PSES a standalone, so it doesn't need to be build by the VSCode client, and as a first step to being used with other clients, and ideally being released onto the PowerShell Gallery.

In that case, in Start-EditorServices.ps1, we run Import-Module PowerShellEditorServices (here), which might end up using a different EditorServices module if one exists further up the path.

Should we change it to ensure it only loads our particular EditorServices module?

TylerLeonhardt · 2018-04-18T21:01:27Z

So, the thing is, Start-EditorServices uses a path passed into it (called BundledModulePath) which gets tacked on to the PSModulePath for only that session.

So this means that, if someone was using PSES for another editor, say vim, they would still put PSES somewhere not on the PSModulePath, run Start-EditorServices and pass in that location. The location would be tacked on to the PSModulePath in that particular session and so Import-Module PowerShellEditorServices would work as intended because it'd be the only one in their PSModulePath.

All that said, I agree we eventually want to put PSES in the Gallery to be installable from Install-Module but to do that, I think we need to figure out what to do with Start-EditorServices.ps1 - for example, do we want to add that to the PowerShellEditorServices module, or have it in its own module, or something like that.

TylerLeonhardt · 2018-04-18T21:03:46Z

that last paragraph could be a separate PR IMO.

rjmholt · 2018-04-18T21:15:46Z

Agreed about the last paragraph. For now I just want to be able to build PSES by itself -- running it might be complicated, but at least we don't need the VSCode client codebase to do it.

As in, I think that should be the scope of this PR.

TylerLeonhardt · 2018-04-18T21:18:39Z

This PR also allows the tests to be run & debugged locally which is great :)

TylerLeonhardt · 2018-04-22T00:07:55Z

Looks like this is good to go but I wanna merge in #629 first because I'd rather you have merge conflicts then them (not that I'm expecting any).

rjmholt · 2018-04-22T00:29:35Z

Yes agreed

code for tests. Change test module path so it works Generalise module path resolution Add module parse error, remove test debug code Use simpler module loading API Import ScriptAnalyzer based on path precedence, not version Remove PSES installing itself, remove zip file Remove -EditorServicesVersion reference in ServerTestBase.cs Move Start-PSES script to new location Update testing Start-PSES script path Change URL of Start-PSES script in comment

rjmholt requested review from TylerLeonhardt and rkeithhill March 29, 2018 22:24

TylerLeonhardt approved these changes Mar 30, 2018

View reviewed changes

rjmholt mentioned this pull request Mar 31, 2018

Migrate VSCode to using EditorServices as a standalone, and building that way PowerShell/vscode-powershell#1252

Merged

TylerLeonhardt reviewed Apr 5, 2018

View reviewed changes

rjmholt commented Apr 5, 2018

View reviewed changes

TylerLeonhardt reviewed Apr 10, 2018

View reviewed changes

TylerLeonhardt reviewed Apr 18, 2018

View reviewed changes

rjmholt changed the title ~~Add module restore build rule~~ Change PSES to be buildable as a standalone Apr 18, 2018

rjmholt force-pushed the import-submodules-build branch from 66dab3e to 6fff81c Compare April 24, 2018 17:34

rjmholt merged commit 6fff81c into PowerShell:master Apr 24, 2018

rjmholt deleted the import-submodules-build branch December 12, 2018 06:00

Change PSES to be buildable as a standalone #647

Change PSES to be buildable as a standalone #647

Conversation

rjmholt commented Mar 29, 2018

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt commented Mar 31, 2018

rjmholt commented Mar 31, 2018

rjmholt commented Apr 4, 2018

TylerLeonhardt 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

Choose a reason for hiding this comment

rjmholt Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt commented Apr 18, 2018

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

rjmholt commented Apr 18, 2018

TylerLeonhardt commented Apr 18, 2018

TylerLeonhardt commented Apr 18, 2018

rjmholt commented Apr 18, 2018

rjmholt commented Apr 18, 2018 • edited Loading

TylerLeonhardt commented Apr 18, 2018

TylerLeonhardt commented Apr 18, 2018

rjmholt commented Apr 18, 2018 • edited Loading

TylerLeonhardt commented Apr 18, 2018 • edited Loading

TylerLeonhardt commented Apr 22, 2018

rjmholt commented Apr 22, 2018

rjmholt Apr 5, 2018 •

edited

Loading

rjmholt Apr 5, 2018 •

edited

Loading

rjmholt Apr 6, 2018 •

edited

Loading

rjmholt commented Apr 18, 2018 •

edited

Loading

rjmholt commented Apr 18, 2018 •

edited

Loading

TylerLeonhardt commented Apr 18, 2018 •

edited

Loading