GeezerVison

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

GeezerVison is showing github.com — Update packaging to only package PowerShell binaries when packaging symbols by TravisEz13 · Pull Request #5145 · PowerShell/PowerShell

Conversation

Travis Plunk (TravisEz13) commented Oct 17, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

disallow all other package types for symbols, as this package is no longer intended to be installed.

I run compliance tools on all binaries in this packages and we are getting more issues from the dependencies than from our product binaries.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Update packaging script to only package PowerShell binaries when crea…

…ting symbols zip. disallow all other package types for symbols

Travis Plunk (TravisEz13) requested review from Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners

October 17, 2017 18:55

Travis Plunk (TravisEz13) commented Oct 17, 2017

Copy link

Copy Markdown

Member Author

https://github.com/PowerShell/PowerShell/pull/5145/files?w=1 make this a lot easier to review.

All reactions

Sorry, something went wrong.

Uh oh!

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

Travis Plunk (TravisEz13) assigned Aditya Patwardhan (adityapatwardhan)

Oct 18, 2017

Dongbo Wang (daxian-dbw) requested changes

Oct 18, 2017

View reviewed changes

Comment thread

tools/packaging/packaging.psm1

		elseif(-not $IncludeSymbols.IsPresent -and $Script:Options.CrossGen) {
		$crossGenCorrect = $true
		}
		elseif ($IncludeSymbols.IsPresent) {

Dongbo Wang (daxian-dbw) Oct 18, 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.

So now it's OK to specify -IncludeSymbols while build with -Crossgen?

Sorry, something went wrong.

Uh oh!

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

All reactions

Travis Plunk (TravisEz13) Oct 18, 2017

Copy link

Copy Markdown

Member 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.

Yes, the non-publish builds always keep the symbols, unaltered.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

tools/packaging/packaging.psm1 Outdated

		log 'setting IncludeSymbols'
		$IncludeSymbols = $PSBoundParameters['IncludeSymbols']
		}
		log "$($IncludeSymbols.IsPresent):$IncludeSymbols"

Dongbo Wang (daxian-dbw) Oct 18, 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.

When -IncludeSymbols is not specified, this log will print just a colon.

Sorry, something went wrong.

Uh oh!

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

All reactions

Travis Plunk (TravisEz13) Oct 18, 2017

Copy link

Copy Markdown

Member 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'll remove... this was troubleshooting...

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

tools/packaging/packaging.psm1

		{
		$tempPath = [System.IO.Path]::GetTempPath()

		$tempFolder = Join-Path -Path $tempPath -ChildPath ([System.IO.Path]::GetRandomFileName())

Aditya Patwardhan (adityapatwardhan) approved these changes

Oct 18, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) 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.

With minor comment

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

tools/packaging/packaging.psm1 Outdated

		}
		if($IncludeSymbols.IsPresent)
		{
		Remove-Item -Path $Source -Recurse -Force

Aditya Patwardhan (adityapatwardhan) Oct 18, 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.

Maybe -ErrorAction SilentlyContinue?

Sorry, something went wrong.

Uh oh!

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

All reactions

Travis Plunk (TravisEz13) Oct 18, 2017

Copy link

Copy Markdown

Member 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.

added

Sorry, something went wrong.

Uh oh!

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

All reactions

Travis Plunk (TravisEz13) added 2 commits

October 18, 2017 12:35


          remove instrumentation used for debugging during development

54b2049


          Add error action silently continue to cleanup for reliability.

25a0a27

Aditya Patwardhan (adityapatwardhan) commented Oct 18, 2017

Copy link

Copy Markdown

Member

Dongbo Wang (@daxian-dbw) can you update your review?

All reactions

Sorry, something went wrong.

Uh oh!

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

Dongbo Wang (daxian-dbw) approved these changes

Oct 20, 2017

View reviewed changes

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.

Approved with 2 comments.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

tools/packaging/packaging.psm1

		{
		$tempPath = [System.IO.Path]::GetTempPath()

		$tempFolder = Join-Path -Path $tempPath -ChildPath ([System.IO.Path]::GetRandomFileName())

Dongbo Wang (daxian-dbw) Oct 20, 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.

I know, but you can use that name to create a folder. The two lines of script shown here are doing the same thing.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

tools/packaging/packaging.psm1

		}
		if($IncludeSymbols.IsPresent)
		{
		Remove-Item -Path $Source -Recurse -Force -ErrorAction SilentlyContinue

Dongbo Wang (daxian-dbw) Oct 20, 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.

It would be great if you can put a comment here saying that "$Source is point to a temporary folder when -IncludeSymbols is present".

Sorry, something went wrong.

Uh oh!

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

All reactions


          Add comment about why folder is being deleted.

d66e3a9

Dongbo Wang (daxian-dbw) approved these changes

Oct 20, 2017

View reviewed changes

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

Aditya Patwardhan (adityapatwardhan) merged commit 14af252 into PowerShell:master

Oct 23, 2017

Travis Plunk (TravisEz13) deleted the fixCompliancePackage branch

December 2, 2017 02:16

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Update packaging to only package PowerShell binaries when packaging s…

4ba1226

…ymbols (PowerShell#5145)

* Update packaging script to only package PowerShell binaries when creating symbols zip.  disallow all other package types for symbols

* remove instrumentation used for debugging during development

* Add error action silently continue to cleanup for reliability.

* Add comment about why folder is being deleted.

Reviewers

Dongbo Wang (daxian-dbw)

daxian-dbw approved these changes

Aditya Patwardhan (adityapatwardhan)

adityapatwardhan approved these changes

Uh oh!

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

Assignees

Aditya Patwardhan (adityapatwardhan)

Labels

None yet

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.

3 participants

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.

Conversation

Travis Plunk (TravisEz13) commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Travis Plunk (TravisEz13) commented Oct 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aditya Patwardhan (adityapatwardhan) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented Oct 18, 2017

Uh oh!

Dongbo Wang (daxian-dbw) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Travis Plunk (TravisEz13) commented Oct 17, 2017 •

edited

Loading

Uh oh!

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