-
Notifications
You must be signed in to change notification settings - Fork 8k
Include the newer Microsoft.PowerShell.ThreadJob module - fixes #19742 #19744
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.
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
Conversation
|
Raised this in this way as to allow for discussion on whether we should remove the currently shipped |
|
looking into the failures now |
|
Doing a quick search on GitHub there are no references to the module qualified |
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
|
The test failure is one that I've seen come up before and has nothing to do with this PR |
|
I think there will need to be a minor update to docs for this PR |
|
this pr renamed module from ThreadJob to Microsoft.PowerShell.ThreadJob? Will this cause duplicate command names In multiple versions of pwsh ? What are the differences between the two modules? |
| "FileType": "NonProduct" | ||
| }, | ||
| { | ||
| "Pattern": "Modules/Microsoft.PowerShell.ThreadJob/Microsoft.PowerShell.ThreadJob.pdb", |
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.
We should avoid shipping the pdb. Can we file an issue in the threadjob repo
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.
Issue filed in the PowerShell/ThreadJob repo
Looking into the repo and it's structure to see if can get a better understanding and potentially raise a PR myself for this
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.
See PR: PowerShell/ThreadJob#29
|
Maintainers would like the Committee to review this breaking change of renaming the module from ThreadJob to Microsoft.PowerShell.ThreadJob. |
| <PackageReference Include="ThreadJob" Version="2.0.3" /> | ||
| <PackageReference Include="Microsoft.PowerShell.ThreadJob" Version="2.1.0" /> |
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.
We should not have both of them as the cmdlet names are same and autoloading might load a different module on different runs.
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.
Maybe a Module Alias feature would help resolve this issue and others like it?
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.
@JosephColvin I've raised about exactly that possible enhancement in #19745 but that won't come in anytime soon I wouldn't think.
|
Currently waiting on this PR in the ThreadJob repo PowerShell/ThreadJob#29 so can then push update to this PR to resolve the conversation above by @adityapatwardhan |
|
Still waiting on the committee to make a conclusion. |
|
The @PowerShell/powershell-committee discussed this (6/28/23) and suggest that proper way to achieve this is as follows:
By taking step 1, we can avoid a breaking change and provide users a glide path to get off the old ThreadJob module which we would want to remove in PS7.6 (our next LTS after 7.4) Implementation note: A new issue should be opened to track the removal of the ThreadJob module in PS7.6. |
|
@JamesWTruher - Just so that I am clear from the above Currently we are waiting on a new ThreadJob release -> then can update this PR with that version number & inc the Microsoft.PowerShell.ThreadJob as currently proposed & all's good for the time being? |
sort of. We want to be sure that the current ThreadJob module is updated with a deprecation notice (and create a proxy from But essentially, we will ship the old explicitly importing the new |
|
Thanks for clarifying @JamesWTruher that was how I understood it from your comment 🙂 |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@JamesWTruher A few questions:
|
|
|
Yes, this PR is blocked until someone can develop the ThreadJob proxy module (could be part of this PR as it's just a script module). |
|
The proxy module shouldn't be developed in this PR, as the code should be in a different repo. |
|
James,
PowerShell will auto load Microsoft.PowerShell.ThreadJob because of the alphabetical order being above ThreadJob (M > T), correct?
Joseph Colvin
IT Coordinator
Warwick Public School<https://www.warwick.k12.nd.us/>
“So you never know who you touch. You never know how or when you'll have an impact, or how important your example can be to someone else" ― Denzel Washington, A Hand to Guide Me
"When men speak ill of you, live so as nobody may believe them." ― Plato
“time, man....time is an illusion. The river flows at an uneven rate but we all drift along in it without noticing until we look back.” – John Gieser
Save a tree, think before you print.
***@***.***?anonymous&ep=signature> Book time to meet with ***@***.***?anonymous&ep=signature>
…________________________________
|
The proxy module is really only relevant as part of PS7 due to the breaking change of ThreadJob being included in PS7 prior to renaming to Microsoft.PowerShell.ThreadJob. It's really only to provide compat for one (or two) release and provide a warning message then we can remove it. If the issue being fixed is critical, we could also consider releasing a new version of Microsoft.PowerShell.ThreadJob as ThreadJob just to be included in PS7. |
|
But Once the proxy module is published, PowerShell just need to pull it during the build, and then it's time to include |
|
@daxian-dbw I agree it would make sense to have the ThreadJob module on PSGallery also indicate being deprecated and using Microsoft.PowerShell.ThreadJob instead. In that case, I would also agree that proxy module would be done outside of this PR and repo since it's published to PSGallery. It would make sense to have that proxy developed in https://github.com/powershell/threadjob
|
|
Opened PowerShell/ThreadJob#30 to track the proxy module work. I will close this PR for now. |
PR Summary
Fixes #19742 by updating the package reference file for modules pulled from the gallery to also include the renamed
Microsoft.PowerShell.ThreadJobmodulePR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).