Fix `build.psm1` to not specify both version and quality for `dotnet-install` #17589

tamasvajk · 2022-06-27T14:37:48Z

The latest changes in dotnet-install enforce that not both of version and quality are specified. See the changes in https://github.com/dotnet/install-scripts/commits/main/src/dotnet-install.sh. Specifically this line.

I haven't done any major testing in powershell context, just ran the following on MacOS:

./dotnet-install.sh -v 7.0.100-preview.2.22153.17 -q signed fails
./dotnet-install.sh -v 7.0.100-preview.2.22153.17 doesn't fail.

PR Checklist

…et-install`

tamasvajk · 2022-06-27T14:39:26Z

This is the reason why the following actions were failing: https://github.com/PowerShell/PowerShell/actions/workflows/codeql-analysis.yml

And with this change applied the action is not failing: https://github.com/PowerShell/PowerShell/actions/runs/2569947207

GitHub
Actions · PowerShell/PowerShell
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

SteveL-MSFT · 2022-06-27T16:46:30Z

Based on dotnet/install-scripts#287 it looks like the expectation is that only --version or --quality should be specified and if both it will error.

SteveL-MSFT

LGTM

SteveL-MSFT · 2022-06-27T17:38:25Z

The Linux test is failing on a tab completion test. I think normally I would recommend that being in a separate PR, but this issue w/ dotnet-install.sh and the tabcompletion issue will need to be taken together to unblock CI.

In ./test/powershell/host/TabCompletion/TabCompletion.Tests.ps1 on line 496, it's checking for a specific ordering of completions and that order appears to have changed. I don't think the test should rely on the ordering. So instead, I would recommend changing that line to:

        $res.CompletionMatches.CompletionText | Should -Contain 'Position'

and do that in this PR.

ghost · 2022-06-27T21:48:32Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ tamasvajk sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

pull-request-quantifier-deprecated · 2022-06-27T21:48:42Z

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

Quantification details

Label      : Extra Small
Size       : +4 -2
Percentile : 2.4%

Total files changed: 2

Change summary by file extension:
.psm1 : +1 -1
.ps1 : +3 -1

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.

daxian-dbw

LGTM

SteveL-MSFT · 2022-06-27T22:23:46Z

@tamasvajk it appears you haven't signed the CLA yet so we can't accept this contribution until that happens

adityapatwardhan · 2022-06-28T01:37:52Z

It seems it was correct for Windows all along:

PowerShell/build.psm1

Lines 1901 to 1907 in 735096b

    
           $installArgs = @{} 
        
           if ($Version) { 
        
               $installArgs += @{ Version = $Version } 
        
           } elseif ($Channel) { 
        
               $installArgs += @{ Quality = $Quality } 
        
               $installArgs += @{ Channel = $Channel } 
        
           }

ghost · 2022-07-18T21:42:16Z

🎉v7.3.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

daxian-dbw · 2022-08-03T18:37:31Z

/backport to release/v7.2.6

github-actions · 2022-08-03T18:37:53Z

Started backporting to release/v7.2.6: https://github.com/PowerShell/PowerShell/actions/runs/2791667878

GitHub
Fix `build.psm1` to not specify both version and quality for `dotnet-install` · PowerShell/PowerShell@ef5e36d
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

github-actions · 2022-08-03T18:38:07Z

@daxian-dbw backporting to release/v7.2.6 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Update `build.psm1` to not specify both version and quality for `dotnet-install`
Applying: Fix failing tab completion test
Using index info to reconstruct a base tree...
M	test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1
Falling back to patching base and 3-way merge...
Auto-merging test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1
CONFLICT (content): Merge conflict in test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Fix failing tab completion test
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

daxian-dbw · 2022-08-03T18:38:43Z

/backport to release/v7.0.12

github-actions · 2022-08-03T18:39:06Z

Started backporting to release/v7.0.12: https://github.com/PowerShell/PowerShell/actions/runs/2791674422

GitHub
Fix `build.psm1` to not specify both version and quality for `dotnet-install` · PowerShell/PowerShell@ef5e36d
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

github-actions · 2022-08-03T18:39:22Z

@daxian-dbw backporting to release/v7.0.12 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Update `build.psm1` to not specify both version and quality for `dotnet-install`
Using index info to reconstruct a base tree...
M	build.psm1
Falling back to patching base and 3-way merge...
Auto-merging build.psm1
CONFLICT (content): Merge conflict in build.psm1
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Update `build.psm1` to not specify both version and quality for `dotnet-install`
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

…install` (#17589)

adityapatwardhan · 2022-08-04T14:07:54Z

Manual backport for 7.2.6 done.

ghost · 2022-08-11T22:18:40Z

🎉v7.2.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

…install` (#17589)

…install` (PowerShell#17589)

Update build.psm1 to not specify both version and quality for `dotn…

d013460

…et-install`

tamasvajk requested review from PaulHigin, TravisEz13, adityapatwardhan, anmenaga and daxian-dbw as code owners June 27, 2022 14:37

pull-request-quantifier-deprecated bot added the Extra Small label Jun 27, 2022

ghost assigned adityapatwardhan Jun 27, 2022

SteveL-MSFT approved these changes Jun 27, 2022

View reviewed changes

Fix failing tab completion test

8b1e21d

daxian-dbw approved these changes Jun 27, 2022

View reviewed changes

daxian-dbw assigned daxian-dbw and unassigned adityapatwardhan Jun 27, 2022

adityapatwardhan approved these changes Jun 28, 2022

View reviewed changes

daxian-dbw merged commit b88cd3a into PowerShell:master Jun 28, 2022

daxian-dbw mentioned this pull request Jun 28, 2022

Fix dotnet-install and a tab completion test #17599

Closed

7 tasks

SeeminglyScience added the CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log label Jul 14, 2022

adityapatwardhan added the BackPort-7.0.x-Approved label Aug 3, 2022

adityapatwardhan added the Backport-7.2.x-Approved label Aug 3, 2022

adityapatwardhan pushed a commit that referenced this pull request Aug 4, 2022

Fix build.psm1 to not specify both version and quality for `dotnet-…

896c427

…install` (#17589)

adityapatwardhan added Backport-7.2.x-Done and removed Backport-7.2.x-Approved labels Aug 8, 2022

adityapatwardhan pushed a commit that referenced this pull request Aug 12, 2022

Fix build.psm1 to not specify both version and quality for `dotnet-…

d29f7a7

…install` (#17589)

TravisEz13 mentioned this pull request Sep 30, 2022

Move ApiScan to compliance build #18191

Merged

22 tasks

TravisEz13 pushed a commit to TravisEz13/PowerShell that referenced this pull request Oct 12, 2022

Fix build.psm1 to not specify both version and quality for `dotnet-…

3488aca

…install` (PowerShell#17589)

TravisEz13 mentioned this pull request Oct 12, 2022

Back-port to 7.0: Fix build.psm1 to not specify both version and quality for dotnet-install #18267

Merged

22 tasks

TravisEz13 added Backport-7.0.x-Done and removed BackPort-7.0.x-Approved labels Oct 13, 2022

TravisEz13 mentioned this pull request Apr 16, 2024

WIP: add more detail to suspicious scriptblock logging #21475

Closed

TravisEz13 mentioned this pull request Jul 9, 2024

Add script to update SDK for release branches #24034

Merged

Fix build.psm1 to not specify both version and quality for dotnet-install #17589

Fix build.psm1 to not specify both version and quality for dotnet-install #17589

Uh oh!

Conversation

tamasvajk commented Jun 27, 2022

PR Checklist

Uh oh!

tamasvajk commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Jun 27, 2022

Uh oh!

ghost commented Jun 27, 2022

Uh oh!

pull-request-quantifier-deprecated bot commented Jun 27, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Jun 27, 2022

Uh oh!

adityapatwardhan commented Jun 28, 2022

Uh oh!

ghost commented Jul 18, 2022

Uh oh!

daxian-dbw commented Aug 3, 2022

Uh oh!

github-actions bot commented Aug 3, 2022 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 3, 2022

Uh oh!

daxian-dbw commented Aug 3, 2022

Uh oh!

github-actions bot commented Aug 3, 2022 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 3, 2022

Uh oh!

adityapatwardhan commented Aug 4, 2022

Uh oh!

ghost commented Aug 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix `build.psm1` to not specify both version and quality for `dotnet-install` #17589

Fix `build.psm1` to not specify both version and quality for `dotnet-install` #17589

tamasvajk commented Jun 27, 2022 •

edited

Loading

SteveL-MSFT commented Jun 27, 2022 •

edited

Loading

github-actions bot commented Aug 3, 2022 •

edited by unfurl-links bot

Loading

github-actions bot commented Aug 3, 2022 •

edited by unfurl-links bot

Loading