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 — Add dependency nuget instructions for PowerShell.Core.Instrumentation resource binary by dantraMSFT · Pull Request #5396 · PowerShell/PowerShell

Conversation

@dantraMSFT

@dantraMSFT Dan Travison (dantraMSFT) commented Nov 9, 2017

Copy link
Copy Markdown
Contributor

Fix: #4939

This PR adds a dependency on PowerShell.Core.Instrumentation.dll to System.Management.Automation.csproj.

This is a native, resource-only binary containing ETW resource manifest and associated strings.

internals.md documents the steps for creating the nuget package and PowerShell.Core.Instrumentation.nuspec contains the template nuget spec.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dantraMSFT
Add dependency nuget packaging instructions for PowerShell.Core.Instr…
c90eaf5 
…umentation ETW resource binary.
@dantraMSFT
Dan Travison (dantraMSFT) requested review from Steve Lee (SteveL-MSFT) and Dongbo Wang (daxian-dbw) November 9, 2017 20:21
@dantraMSFT
Dan Travison (dantraMSFT) requested review from Travis Plunk (TravisEz13) and Aditya Patwardhan (adityapatwardhan) as code owners November 9, 2017 20:21
@dantraMSFT
Fix markdown MD031 Fenced code blocks errors.
74bdbb3
@dantraMSFT

Dan Travison (dantraMSFT) commented Nov 9, 2017

Copy link
Copy Markdown
Contributor Author

NOTE: Merging is dependent on the PowerShell.Core.Instrumentation nuget package being published. I'll update the thread when it is ready.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Dan Travison (dantraMSFT) added 2 commits November 9, 2017 13:17
@dantraMSFT
Fix MD022 Headers should be surrounded by blank lines
8902527
@dantraMSFT
Add a note about Authenticode Dual-signing requirement.
da71450
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Nov 9, 2017
View reviewed changes
Comment thread docs/building/internals.md Outdated

To successfully decode PowerShell Core ETW events, The manifest and resource binary need to be registered on the system.

To create a new NuGet package for `PowerShell.Core.Instrumentation.dll`, you will need the PowerShell.Core.Instrumentation.nuspec found in the repo under src\PowerShell.Core.Instrumentation.

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 9, 2017

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.

PowerShell.Core.Instrumentation.nuspec should be put in the code block ...

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread docs/building/internals.md Outdated
│ └── PowerShell.Core.Instrumentation.dll
```

NOTE: Since these are native binaries used on Windows, they need to be AuthenticodeDual signed, certificate code: 402 before creating the nuget package.

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 9, 2017

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.

certificate code: 402

per my discussion with Travis, this code should not be exposed in public. I think only mentioning "need to be AuthenticodeDual signed" should be good enough.

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/System.Management.Automation.csproj
<PackageReference Include="System.Security.Permissions" Version="4.4.0" />
<PackageReference Include="System.Text.Encoding.CodePages" Version="4.4.0" />
<PackageReference Include="Microsoft.Management.Infrastructure" Version="1.0.0-alpha*" />
<PackageReference Include="PowerShell.Core.Instrumentation" Version="6.0.0-beta*" />

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 9, 2017

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.

Is it depended by System.Management.Automation.dll or powershell as a whole? Put it another way, if an application is hosting System.Management.Automation.dll only, does it need the resource dll to work properly?

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

None of the code has a direct dependency on the binary. I chose SMA to pull the nuget package since it contains the event raising code. If the dll is not present or the manifest isn't registered, PowerShell will continue to work without issue but the event log and custom consumers won't be able to decode the events.

Sorry, something went wrong.

Uh oh!

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

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 10, 2017

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.

Thanks for the clarification. #Close

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread docs/building/internals.md Outdated

NOTE: Since these are native binaries used on Windows, they need to be AuthenticodeDual signed, certificate code: 402 before creating the nuget package.

Lastly, run `nuget pack` from teh root of the repo. The following command creates the nuget package from the c:\mypackage directory and places the nuget package in .\src\powershell-win-core. Note that you may need the latest `nuget.exe`.

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 9, 2017

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.

run nuget pack from teh root of the repo

Type teh.

How about changing it to "Lastly, run the following command from the root of repo."?

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread docs/building/internals.md Outdated

```powershell
nuget pack .\src\PowerShell.Core.Instrumentation\PowerShell.Core.Instrumentation.nuspec -BasePath c:\mypackage -OutputDirectory .\src\powershell-win-
core

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 9, 2017

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.

'core' should not be in a new line.

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread docs/building/internals.md Outdated

### PowerShell.Core.Instrumentation

To successfully decode PowerShell Core ETW events, The manifest and resource binary need to be registered on the system.

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 9, 2017

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.

events, The manifest

typo.

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread docs/building/internals.md Outdated
<version>6.0.0-RC</version>
```

Next, create the directory structure needed for the contents of the nuget packagestructure. The final directory and file layout is listed below.

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 9, 2017

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.

packagestructure

typo

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dantraMSFT
PR Feedback.
ed2c484
iSazonov
Ilya (iSazonov) reviewed Nov 10, 2017
View reviewed changes
Comment thread src/PowerShell.Core.Instrumentation/PowerShell.Core.Instrumentation.nuspec
<authors>Microsoft</authors>
<owners>Microsoft</owners>
<requireLicenseAcceptance>false</requireLicenseAcceptance>
<description>PowerShell Core ETW resource binary</description>

@iSazonov Ilya (iSazonov) Nov 10, 2017

Copy link
Copy Markdown
Collaborator

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.

Should we remove "Core"? It seems we did this previously.

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

Hmm, we use PowerShell Core all across our .MD files in the repo. (547 times). I figured I'd follow that pattern as well as ensure that there is no confusion between this and Windows PowerShell. That's also the reason I selected the assembly name.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/PowerShell.Core.Instrumentation/PowerShell.Core.Instrumentation.nuspec Outdated
<owners>Microsoft</owners>
<requireLicenseAcceptance>false</requireLicenseAcceptance>
<description>PowerShell Core ETW resource binary</description>
<copyright>Copyright 2017 Microsoft</copyright>

@iSazonov Ilya (iSazonov) Nov 10, 2017

Copy link
Copy Markdown
Collaborator

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 use the string (c) Microsoft Corporation. All rights reserved.

Also should we start all XML tags with capitalization?

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

I did a survey of all nuspec files in my package cache, and they all use lowercase for the xml elements.

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

Fixed the copyright text

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Nov 10, 2017
View reviewed changes
Comment thread docs/building/internals.md Outdated

NOTE: Since these are native binaries used on Windows, they need to be Authenticode Dual signed before creating the nuget package.

Lastly, run the following command from the root of the repo to create the nuget package. The nuget package is placed `.\src\powershell-win-core`. Note that you may need the latest `nuget.exe`.

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 10, 2017

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.

The nuget package is placed .\src\powershell-win-core

Maybe "is placed at"?

Sorry, something went wrong.

Uh oh!

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

@dantraMSFT Dan Travison (dantraMSFT) Nov 10, 2017

Copy link
Copy Markdown
Contributor Author

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.

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw Dongbo Wang (daxian-dbw) assigned Aditya Patwardhan (adityapatwardhan) Nov 10, 2017
Dan Travison (dantraMSFT) added 2 commits November 10, 2017 15:12
@dantraMSFT
Fix grammar
f60a430
@dantraMSFT
Update copyrigth value
8b150a5
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Nov 10, 2017
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
@daxian-dbw Dongbo Wang (daxian-dbw) added this to the 6.0.0-RC milestone Nov 10, 2017
@dantraMSFT
Set version to Beta.10
51f99f7
@dantraMSFT

Dan Travison (dantraMSFT) commented Nov 11, 2017

Copy link
Copy Markdown
Contributor Author

FYI: I updated the nuspec to refer to beta.10
All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Nov 11, 2017

Copy link
Copy Markdown
Collaborator

What is next release? Beta.10 or RC.1 ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Nov 11, 2017

Copy link
Copy Markdown
Member

Ilya (@iSazonov) Dan Travison (@dantraMSFT) misspoke, next release is RC
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 Nov 11, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Ilya (@iSazonov) Dan Travison (@dantraMSFT) was referring to the version of the nuget package PowerShell.Core.Instrumentation, not the next PowerShell release.

After publishing the beta.9 PowerShell.Core.Instrumentation we learnt that resource DLL also needs to be signed, and that's why the beta.10 package was published shortly after.
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 Nov 11, 2017

Copy link
Copy Markdown
Member

Dan Travison (@dantraMSFT) There are spelling errors in the .md file. Please see the failed Linux CI.
For file name and file path, you can put them in code block to avoid the spelling check.
For other spelling errors, see https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#spellchecking-documentation about how to add them to the .spelling file. When adding a new word to .spelling, please make sure the section that contains the new word is sorted ascendingly (each existing section should have been sorted).
All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Nov 11, 2017

Copy link
Copy Markdown
Collaborator

It's confusing. Shouldn't we be using the same version for all packages? It seems .Net Core do so.
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 Nov 11, 2017

Copy link
Copy Markdown
Member

I agree. That's something we should consider to do. We currently use inconsistent versions for nuget packages that PowerShell depends on, such as pspr.windows, psrp, libmi, MMI and powershell.core.instrument. It would be good if we can unify them before GA.
iSazonov and adityapatwardhan reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

Dan Travison (dantraMSFT) commented Nov 12, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

libmi is not owned by PowerShell, it is part if omi and it's version is tied to it. Psrp has a build dependency on both libmi and omi so I tied the version to it. It has no dependency on PowerShell and will limely be reved independent of it so I see no reason to version it with PowerShell
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 Nov 12, 2017

Copy link
Copy Markdown
Member

Good point. So maybe just those nuget packages that are owned by PowerShell itself.

Dan Travison (@dantraMSFT) Please fix the spelling errors in Travis-CI build. See my comment #5396 (comment) for more info. We cannot merge the PR with the failed CI.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dantraMSFT
Fix spelling in the document and update .spelling file.
ed7fad9
@dantraMSFT

Dan Travison (dantraMSFT) commented Nov 12, 2017

Copy link
Copy Markdown
Contributor Author

Dongbo Wang (@daxian-dbw), I fixed spelling issues; updated .spelling with new entries (authenticode and PowerShell.Core.Instrumentation)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Nov 13, 2017
View reviewed changes
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Nov 13, 2017

Copy link
Copy Markdown
Member

Dan Travison (@dantraMSFT) Just to confirm, the nuget package has been published right?
All reactions

Sorry, something went wrong.

Uh oh!

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

Dan Travison (dantraMSFT) commented Nov 13, 2017

Copy link
Copy Markdown
Contributor Author

Aditya Patwardhan (@adityapatwardhan): Yes, the nuget package has been published.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit d5a974f into PowerShell:master Nov 13, 2017
@dantraMSFT
Dan Travison (dantraMSFT) deleted the dantra/issue4939 branch November 13, 2017 20:19
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@dantraMSFT @adityapatwardhan
Add dependency nuget instructions for PowerShell.Core.Instrumentation…
c14632e 
… resource binary (PowerShell#5396)

* Add dependency nuget packaging instructions for PowerShell.Core.Instrumentation ETW resource binary.

* Fix markdown MD031 Fenced code blocks errors.

* Fix MD022 Headers should be surrounded by blank lines

* Add a note about Authenticode Dual-signing requirement.

* PR Feedback.

* Set version to Beta.10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
@TravisEz13 Travis Plunk (TravisEz13) Awaiting requested review from TravisEz13

Uh oh!

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

Assignees

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

None yet

Projects

None yet

Milestone

6.0.0-RC

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

5 participants

@dantraMSFT @iSazonov @SteveL-MSFT @daxian-dbw @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 01402520 — 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.