Skip to content

Comments

Do not ship fullclr binaries of PackageManagement#8700

Merged
TravisEz13 merged 4 commits intoPowerShell:masterfrom
bergmeister:RemovePSGetFullClr
Jan 22, 2019
Merged

Do not ship fullclr binaries of PackageManagement#8700
TravisEz13 merged 4 commits intoPowerShell:masterfrom
bergmeister:RemovePSGetFullClr

Conversation

@bergmeister
Copy link
Contributor

@bergmeister bergmeister commented Jan 19, 2019 •
edited
Loading

PR Summary

Related: #8699

PR Context

Because PowerShellGet does not support publishing/saving module on a per platform basis (see this issue), PowerShell currently also ships the fullclr binaries of PackageManagement, which it does not need. Therefore removing it to minimise the package size, this saves 1.19 MB.

PR Checklist

@bergmeister bergmeister changed the title Remove fullclr binaries of PowerShellGet from WiX installer WIP Remove fullclr binaries of PowerShellGet from WiX installer Jan 19, 2019
@bergmeister bergmeister changed the title WIP Remove fullclr binaries of PowerShellGet from WiX installer WIP Remove fullclr binaries of PackageManagement from WiX installer Jan 19, 2019
@bergmeister bergmeister changed the title WIP Remove fullclr binaries of PackageManagement from WiX installer WIP Do not ship fullclr binaries of PackageManagement Jan 19, 2019
@bergmeister bergmeister changed the title WIP Do not ship fullclr binaries of PackageManagement Do not ship fullclr binaries of PackageManagement Jan 19, 2019
daxian-dbw
daxian-dbw approved these changes Jan 21, 2019
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 21, 2019

I guess that we break in-place upgrade (we will not remove the trash on file system) if removing the components.

@daxian-dbw
Copy link
Member

daxian-dbw commented Jan 21, 2019

Hmm, definitely need @TravisEz13's approval on this.

@bergmeister
Copy link
Contributor Author

bergmeister commented Jan 21, 2019 •
edited
Loading

@iSazonov Why would the MSI upgrade not remove the old component? And even if it did not remove it, then there would still not be a problem since this folder is not needed/useful for PowerShell Core at all. In the related issue #8699, I also describe that the next version of Packagemanagement will ship with a netstandard2.0 folder that replaces the current 2 folders netcoreapp2.0 and netstandard1.6, so we will need to tackle this problem anyway.

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 22, 2019 •
edited
Loading

We already had a problem with the shortcut key in the registry so we should be careful here.
Also we should consider a patch update scenario.

@SteveL-MSFT @TravisEz13 what do you think?

@iSazonov iSazonov requested a review from SteveL-MSFT January 22, 2019 03:25
@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jan 22, 2019

Seems fine to me. Even if the existing assemblies don't get removed, it didn't make it worse.

@TravisEz13
Copy link
Member

TravisEz13 commented Jan 22, 2019

We should not consider a patch upgrade scenario during a preview. This change should not be back ported to 6.1, but it is fine to introduce during the preview process.

@TravisEz13 TravisEz13 merged commit beae569 into PowerShell:master Jan 22, 2019
@adityapatwardhan adityapatwardhan added the CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log label Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

Assignees

@TravisEz13 TravisEz13

Labels

CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@bergmeister @iSazonov @daxian-dbw @SteveL-MSFT @TravisEz13 @adityapatwardhan