Skip to content

Revert 5.1-breaking changes to build\Start-PSBootStrap #16738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
IISResetMe wants to merge 1 commit into from
Closed

Revert 5.1-breaking changes to build\Start-PSBootStrap #16738

IISResetMe wants to merge 1 commit into from

Conversation

@IISResetMe
Copy link
Collaborator

@IISResetMe IISResetMe commented Jan 12, 2022 •
edited
Loading

Fixes #16330

PR Summary

Runtime updates in #16309 broke Start-PSBootbootstrap on Windows PowerShell 5.1 by introducing a hard dependency on the [SemanticVersion] type, introduced in 6.0.

This commit replaces the type reference with a PowerShell class that implements the necessary subset of functionality (namely IComparable) from [SemanticVersion] needed for sorting SDK versions, ostensibly the motivation for introducing this dependency in the first place.

PR Context

I, like many others, have been building PowerShell >=6 from source with Windows PowerShell 5.1 for years and still occassionally do. Having to manually patch build.psm1 everytime I want to bootstrap a build from WPS is frustrating. Hence this PR.

PR Checklist

@IISResetMe
Revert 5.1-breaking changes in build.psm1
5866628 
Runtime updates in PowerShell#16309 broke `Start-PSBootbootstrap` on Windows PowerShell 5.1 by introducing a hard dependency on the `[SemanticVersion]` type, introduced in 6.0.

This commit replaces the type reference with a PowerShell class that implements the necessary subset of functionality from `[SemanticVersion]` needed for sorting SDK versions.
@ghost ghost assigned anmenaga Jan 12, 2022
@pull-request-quantifier-deprecated
Copy link

pull-request-quantifier-deprecated bot commented Jan 12, 2022

This PR has 110 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details 

Label      : Medium
Size       : +108 -2
Percentile : 42%

Total files changed: 1

Change summary by file extension:
.psm1 : +108 -2

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

  • Fast and predictable releases to production:
    • Optimal size changes are more likely to be reviewed faster with fewer
      iterations.
    • Similarity in low PR complexity drives similar review times.
  • Review quality is likely higher as complexity is lower:
    • Bugs are more likely to be detected.
    • Code inconsistencies are more likely to be detetcted.
  • Knowledge sharing is improved within the participants:
    • Small portions can be assimilated better.
  • Better engineering practices are exercised:
    • Solving big problems by dividing them in well contained, smaller problems.
    • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

  • Use the PullRequestQuantifier to quantify your PR accurately
    • Create a context profile for your repo using the context generator
    • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
    • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
    • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
  • Change your engineering behaviors
    • For PRs that fall outside of the desired spectrum, review the details and check if:
      • Your PR could be split in smaller, self-contained PRs instead
      • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

  • One line was added: +1 -0
  • One line was deleted: +0 -1
  • One line was modified: +1 -1 (git diff doesn't know about modified, it will
    interpret that line like one addition plus one deletion)
  • Change percentiles: Change characteristics (addition, deletion, modification)
    of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

@IISResetMe IISResetMe changed the title WIP: Revert 5.1-breaking changes to build\Start-PSBootStrap Revert 5.1-breaking changes to build\Start-PSBootStrap Jan 12, 2022
PaulHigin
PaulHigin reviewed Jan 12, 2022
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

These changes look fine to me. My only concern is that we now would have to maintain this code, and pick up any bug fixes from dotNet. Do people agree this is Ok? I imagine the code is pretty stable, but would still need to be maintained.

@IISResetMe
Copy link
Collaborator Author

IISResetMe commented Jan 12, 2022

These changes look fine to me. My only concern is that we now would have to maintain this code, and pick up any bug fixes from dotNet. Do people agree this is Ok? I imagine the code is pretty stable, but would still need to be maintained.

SemanticVersion seems to have been stable since SemVer 2.0 compatibility was introduced in #5037 (November 2017) - all subsequent changes has been pure refactoring to take advantage of new runtime/language features or comply with updated QA rules - no behavioral changes have been made as far as I can tell.

@daxian-dbw daxian-dbw added the Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers label Jan 13, 2022
@daxian-dbw
Copy link
Member

daxian-dbw commented Jan 13, 2022

I have the similar concern, and thus marked this to be reviewed by maintainer. Also, CIs failed because the build was not successful. I think the failure is related to changes in this PR because I don't see similar failure in other PRs or daily builds.

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 13, 2022

The sdk selection logic is "dumb" - get list, sort, pick up first - we don't search required sdk in the list. So we can replace Get-LatestInstalledSDK with one command dotnet --version.

@IISResetMe
Copy link
Collaborator Author

IISResetMe commented Jan 13, 2022

The sdk selection logic is "dumb" - get list, sort, pick up first - we don't search required sdk in the list. So we can replace Get-LatestInstalledSDK with one command dotnet --version.

I was a bit perplexed by this logic as well.

I assume that also explains why I get "dotnet (6.0.101) is out of date, downloading dotnet (6.0.100)" every time I bootstrap since installing 6.0.101 on my machine ¯\(ツ)/¯

@iSazonov
Copy link
Collaborator

iSazonov commented Jan 13, 2022

I was a bit perplexed by this logic as well.

@IISResetMe
:-) The Build.psm1 is very old and it has long been a monster, there are contradictions on every line.
It should be cleaned up and simplified a long time ago, use the newer features of the latest versions including source generators,
move the build process to csproj files as much as possible, making it more friendly for other communities which could issue their distributives,
move the release process into a separate repository as it is essentially an internal MSFT process.
It's impossible to understand why the MSFT team chooses to roam the jungle year after year, but it's their time. :-)

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jan 18, 2022

Please see comments here: #16330 (comment)

Closing this PR as PowerShell team Maintainer's have consensus, that this is not required.

@adityapatwardhan adityapatwardhan added Resolution-Won't Fix The issue won't be fixed, possibly due to compatibility reason. and removed Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers labels Jan 18, 2022
@IISResetMe
Copy link
Collaborator Author

IISResetMe commented Jan 19, 2022

@adityapatwardhan consensus that it shouldn't be possible to bootstrap a build from Windows PowerShell? Or consensus that there are better ways of maintaining compatibility?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

1 more reviewer

@PaulHigin PaulHigin PaulHigin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@anmenaga anmenaga

Labels

Medium Resolution-Won't Fix The issue won't be fixed, possibly due to compatibility reason.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Start-PSBootstrap fails on PowerShell 5.1

6 participants

@IISResetMe @daxian-dbw @iSazonov @adityapatwardhan @PaulHigin @anmenaga