GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Do not ship fullclr binaries of PackageManagement by bergmeister · Pull Request #8700 · PowerShell/PowerShell

Conversation

@bergmeister

@bergmeister Christoph Bergmeister (bergmeister) commented Jan 19, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] to your commit messages if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bergmeister
[feature] remove fullclr binaries of PowerShellGet from WiX installer
0cf16da
@bergmeister Christoph 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 Christoph 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
do not copy fullclr folder at build time
01499cb
@bergmeister
Christoph Bergmeister (bergmeister) requested review from Paul Higinbotham (PaulHigin), Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan), Andrew (anmenaga) and Dongbo Wang (daxian-dbw) as code owners January 19, 2019 21:46
@bergmeister Christoph 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
remove component references to fix WiX installer
40c7135
@bergmeister Christoph Bergmeister (bergmeister) changed the title WIP Do not ship fullclr binaries of PackageManagement Do not ship fullclr binaries of PackageManagement Jan 19, 2019
@bergmeister
[feature] Empty commit to trigger feature tests now that installer is…
f0c3d06 
… fixed
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Jan 21, 2019
View reviewed changes

@daxian-dbw Dongbo Wang (daxian-dbw) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov

Ilya (iSazonov) commented Jan 21, 2019

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jan 21, 2019

Copy link
Copy Markdown
Member

Hmm, definitely need Travis Plunk (@TravisEz13)'s approval on this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bergmeister

Christoph Bergmeister (bergmeister) commented Jan 21, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Ilya (@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.
daxian-dbw reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Jan 22, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.

Steve Lee (@SteveL-MSFT) Travis Plunk (@TravisEz13) what do you think?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) requested a review from Steve Lee (SteveL-MSFT) January 22, 2019 03:25
@iSazonov Ilya (iSazonov) assigned Travis Plunk (TravisEz13) Jan 22, 2019
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Jan 22, 2019
View reviewed changes
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Jan 22, 2019

Copy link
Copy Markdown
Member

Seems fine to me. Even if the existing assemblies don't get removed, it didn't make it worse.
bergmeister, TravisEz13, and iSazonov reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13

Travis Plunk (TravisEz13) commented Jan 22, 2019

Copy link
Copy Markdown
Member

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.
bergmeister reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TravisEz13
Travis Plunk (TravisEz13) approved these changes Jan 22, 2019
View reviewed changes
@TravisEz13
Travis Plunk (TravisEz13) merged commit beae569 into PowerShell:master Jan 22, 2019
@adityapatwardhan Aditya Patwardhan (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
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@bergmeister @TravisEz13
Do not ship fullclr binaries of PackageManagement (PowerShell#8700)
1ffc29a 
## PR Summary

Related: PowerShell#8699

## PR Context  

Because `PowerShellGet` does not support publishing/saving module on a per platform basis (see [this](https://github.com/PowerShell/PowerShellGet/issues/273) 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 approved these changes
@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Awaiting requested review from adityapatwardhan
@anmenaga Andrew (anmenaga) Awaiting requested review from anmenaga
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@TravisEz13 Travis Plunk (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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@bergmeister @iSazonov @daxian-dbw @SteveL-MSFT @TravisEz13 @adityapatwardhan
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.