Pull PSReadLine from PSGallery #5759

lzybkr · 2017-12-29T20:20:30Z

PR Summary

Instead of building PSReadLine from this repo, pull it from the gallery.

This pulls v2.0 of PSReadLine which does have documented breaking changes from v1.2, but the risk is small - the features that have changed are typically only used in a profile and aren't used all that often anyway.

Fix #996

Hardcodes version of modules pulled from PSGallery

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
NA User facing Documentation needed
- NA Issue filed - Issue link:
Change is not breaking
NA Make sure you've added a new test if existing tests do not effectively test the code changed
- NA Add [feature] if the change is significant or affectes feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

lzybkr · 2017-12-29T22:17:32Z

I tested these changes locally and had no problems building (I didn't run tests though.)

The CI failures show 2 problems:

If restoring a modules fails, tests probably should not run.
We rely on PowerShellGet to build. This is unnecessary, we could use nuget directly.

I'd like some feedback on 2 - should we update PowerShellGet as part of the build, or just use nuget?

TravisEz13 · 2018-01-02T19:43:19Z

build.psm1

This is causing failures

Yes, I know, see my request for feedback here.

Also, We should only do this (restore a PreRelease module) if needed.

TravisEz13 · 2018-01-02T20:07:57Z

It is very difficult to use NuGet directly across platforms. Please update PowerShellGet

SteveL-MSFT · 2018-01-02T20:32:54Z

I think we can take this early for 6.1.0, but we should stick with the released version of PSReadline for 6.0.0-GA

Instead of building PSReadLine from this repo, pull it from the gallery. Fix PowerShell#996

lzybkr · 2018-01-08T17:30:04Z

@TravisEz13 - I tried updating PowerShellGet, but in the end it was easier to use nuget (indirectly, via a csproj) and copy out of the nuget cache.

It looks like everything passes now, so I think this can be merged.

Note that PSReadLine will no longer be cross-gen'd after this PR, so if you merge, I'd open an issue to track adding a call to crossgen for PSReadLine - I'm not sure when I'll get a chance to do that.

TravisEz13 · 2018-01-11T00:52:24Z

build.psm1

+
+        Remove-Item -Force -ErrorAction Ignore -Recurse "$Destination/$name"
+        New-Item -Path $dest -ItemType Directory -Force -ErrorAction Stop > $null
+        $dontCopy = '*.nupkg', '*.nupkg.sha512', '*.nuspec', 'System.Runtime.InteropServices.RuntimeInformation.dll'


This is replicating powershellget behavior which may change over time. I'd rather not do this if we don't have to.

Yeah, I get that, but this was easier to make work and has less impact on the user's environment or build machine.

Also notice how much less code there is doing it this way - I think that's valuable too.

TravisEz13 · 2018-01-11T00:53:19Z

What was the issue with PowerShellGet? Can we investigate and fix that issue?

daxian-dbw · 2018-01-11T00:52:27Z

build.psm1

+    # These modules are restored to the global nuget package cache at the
+    # same time we restore nuget packages to build PowerShell, we just
+    # copy them from the cache to the final layout location now.
+    $modules = @(


$modules seems not used anywhere.

Right, I'll remove it.

daxian-dbw · 2018-01-11T00:56:01Z

src/Microsoft.PowerShell.SDK/Microsoft.PowerShell.SDK.csproj

    <ProjectReference Include="..\Microsoft.PowerShell.ConsoleHost\Microsoft.PowerShell.ConsoleHost.csproj" />
    <ProjectReference Include="..\Microsoft.PowerShell.Security\Microsoft.PowerShell.Security.csproj" />
    <ProjectReference Include="..\System.Management.Automation\System.Management.Automation.csproj" />
+    <ProjectReference Include="..\Modules\PSGalleryModules.csproj" />


When building nuget packages for individual assemblies, I think this reference will cause a nuget package to be generated for PSGalleryModules.csproj and the generated Microsoft.PowerShell.SDK package will have a reference to the PSGalleryModules nuget package. We should avoid creating the unneeded PSGalleryModules nuget package.

Indeed, I'll remove this and do the restore on PSGalleryModules.csproj explicitly.

lzybkr · 2018-01-11T01:22:11Z

PowerShellGet would update successfully, but I would get errors from PackageManagement, presumably because it is a binary module and the newly installed version wasn't being picked up.

I considered "fixing" that by running the update in a child process, but:

Running in a child process can be harder to debug, harder to pass parameters (no live objects e.g.)
I just think it's cleaner to use nuget for the package restore, e.g. the packages are cached, so you don't pull the package every time you build.

lzybkr · 2018-01-11T18:16:43Z

I updated to address the feedback.

I don't plan on changing back to using PowerShellGet - I think this approach could be turned on by default - we can rely on the nuget cache instead of fetching from the network on every build.

Also regarding this approach versus using PowerShellGet - in my opinion:

Either way, we need some logic to remove something, PowerShellGet artifacts or nuget artifacts
The build fails quickly this way if the package is missing - the previous way the build succeeds and you don't discover a missing module until running tests, assuming there are tests.
With a csproj, the dependencies are easier to express, maybe not in the SDK csproj, but the dependency is more discoverable than being hidden in build.psm1.

adityapatwardhan · 2018-01-22T22:26:23Z

Reverting this change as Console tests are failing on non-windows. The failure is: An error has occurred that was not properly handled. Additional information is shown below. The PowerShell process will exit.

This reverts commit beffdcf.

lzybkr requested review from TravisEz13, adityapatwardhan, charub and daxian-dbw as code owners December 29, 2017 20:20

lzybkr force-pushed the pull_psreadline branch from bcc9f11 to 5847e3e Compare December 29, 2017 21:10

TravisEz13 added the Breaking-Change breaking change that may affect users label Jan 2, 2018

TravisEz13 reviewed Jan 2, 2018

View reviewed changes

SteveL-MSFT added this to the 6.1.0-MQ milestone Jan 2, 2018

lzybkr force-pushed the pull_psreadline branch 3 times, most recently from e890b8d to 3f39977 Compare January 8, 2018 05:45

Pull PSReadLine from PSGallery

75bee72

Instead of building PSReadLine from this repo, pull it from the gallery. Fix PowerShell#996

lzybkr force-pushed the pull_psreadline branch from 3f39977 to 75bee72 Compare January 8, 2018 05:58

lzybkr added 2 commits January 7, 2018 22:24

Update version in test

efac558

Fix Linux build

73041df

daxian-dbw assigned TravisEz13 Jan 8, 2018

TravisEz13 reviewed Jan 11, 2018

View reviewed changes

daxian-dbw reviewed Jan 11, 2018

View reviewed changes

Address review feedback

d63185e

lzybkr mentioned this pull request Jan 11, 2018

Linux: When Prompt include Unicode Char, Cursor Positioned Error #5856

Closed

daxian-dbw approved these changes Jan 20, 2018

View reviewed changes

TravisEz13 merged commit beffdcf into PowerShell:master Jan 20, 2018

markekraus mentioned this pull request Jan 21, 2018

Nightly Builds failing after #5759 #5973

Closed

adityapatwardhan mentioned this pull request Jan 22, 2018

Fix update help tests for PSReadline #5980

Closed

5 tasks

adityapatwardhan added a commit that referenced this pull request Jan 22, 2018

Revert "Pull PSReadLine from PSGallery (#5759)"

838ec17

This reverts commit beffdcf.

adityapatwardhan mentioned this pull request Jan 22, 2018

Revert "Pull PSReadLine from PSGallery" #5986

Merged

daxian-dbw mentioned this pull request Feb 6, 2018

Restore modules from the NuGet package cache by using dotnet restore #6111

Merged

9 tasks

lzybkr deleted the pull_psreadline branch June 15, 2018 15:59

Pull PSReadLine from PSGallery #5759

Pull PSReadLine from PSGallery #5759

Uh oh!

Conversation

lzybkr commented Dec 29, 2017 • edited by TravisEz13 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

lzybkr commented Dec 29, 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!

TravisEz13 Jan 2, 2018 • 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!

TravisEz13 commented Jan 2, 2018

Uh oh!

SteveL-MSFT commented Jan 2, 2018

Uh oh!

lzybkr commented Jan 8, 2018

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 commented Jan 11, 2018

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!

lzybkr commented Jan 11, 2018

Uh oh!

lzybkr commented Jan 11, 2018

Uh oh!

adityapatwardhan commented Jan 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lzybkr commented Dec 29, 2017 •

edited by TravisEz13

Loading

lzybkr commented Dec 29, 2017 •

edited

Loading

TravisEz13 Jan 2, 2018 •

edited

Loading