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 — Enable macOS launcher by thezim · Pull Request #5291 · PowerShell/PowerShell

Conversation

@thezim

@thezim Greg Zimmerman (thezim) commented Nov 1, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Notable change this time around.

  • Values set by script are "unset" in Info.plist by default to prevent conflicts if a fresh clone.
  • After packaging CFBundleIdentifier is set to random value to prevent it from being picked up by installer. An empty value could not be used because native defaults doesn't allow it.
  • The magic here is getting macOS to recognize the change in the plist file. plutil is enough to make this happen.

Fixes #5262

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Greg Zimmerman (thezim) added 3 commits October 31, 2017 20:59
@thezim
Add macOS PowerShell launcher.
6602987
@thezim
Add conversion back to XML.
eb5c4dd
@thezim
Remove unnecessary touch; plutil is enough.
3543240
@thezim
Greg Zimmerman (thezim) requested review from Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners November 1, 2017 02:17
@thezim Greg Zimmerman (thezim) mentioned this pull request Nov 1, 2017
Remove the workaround to enable macOS launcher #5262
Closed
@TravisEz13

Travis Plunk (TravisEz13) commented Nov 1, 2017

Copy link
Copy Markdown
Member

Can you go to https://travis-ci.org/profile/thezim?offset=0 and enable your repo then do another push? This will trigger a build with packaging.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw Dongbo Wang (daxian-dbw) assigned Aditya Patwardhan (adityapatwardhan) Nov 1, 2017
@thezim
Correct spelling.
4dc5c8d
@thezim

Greg Zimmerman (thezim) commented Nov 2, 2017

Copy link
Copy Markdown
Contributor Author

Travis Plunk (@TravisEz13) Done.
All reactions

Sorry, something went wrong.

Uh oh!

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

Travis Plunk (TravisEz13) commented Nov 2, 2017

Copy link
Copy Markdown
Member

Verified packaged correctly here: https://travis-ci.org/thezim/PowerShell/builds/296027725
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TravisEz13
Travis Plunk (TravisEz13) reviewed Nov 2, 2017
View reviewed changes
Comment thread tools/packaging/packaging.psm1 Outdated

# Set permissions.
Start-NativeExecution {
find $macosapp | xargs chmod 755

@TravisEz13 Travis Plunk (TravisEz13) Nov 2, 2017 •
edited
Loading

Uh oh!

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

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.

Can you undo this when it's finished? Or remove the folder when it's done?

Sorry, something went wrong.

Uh oh!

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

@thezim Greg Zimmerman (thezim) Nov 3, 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.

Sure thing, good catch, commit added. I realized that 755 wasn't needed for everything so I only set what was absolutely needed and then restore to a default state post fpm.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
TravisEz13
Travis Plunk (TravisEz13) requested changes Nov 2, 2017
View reviewed changes

@TravisEz13 Travis Plunk (TravisEz13) left a comment •
edited
Loading

Uh oh!

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

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.

Can we not leave files with altered permission laying around?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@thezim
Set only permissions needed and restore defaults when done.
12516d8
TravisEz13
Travis Plunk (TravisEz13) approved these changes Nov 3, 2017
View reviewed changes
@TravisEz13

Travis Plunk (TravisEz13) commented Nov 3, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

packaging built correctly here: https://travis-ci.org/thezim/PowerShell/jobs/296567451
All reactions

Sorry, something went wrong.

Uh oh!

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

Travis Plunk (TravisEz13) commented Nov 3, 2017

Copy link
Copy Markdown
Member

relaunched AppVeyor due to myget failure..
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Nov 3, 2017
View reviewed changes
Comment thread tools/packaging/packaging.psm1 Outdated
$newPackagePath = Join-Path $createdPackage.DirectoryName $newPackageName
$createdPackage = Rename-Item $createdPackage.FullName $newPackagePath -PassThru -Force:$Force
}
if($pscmdlet.ShouldProcess("Cleanup macOS launcher"))

@daxian-dbw Dongbo Wang (daxian-dbw) Nov 3, 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.

Should this cleanup code be moved to the if ($Environment.IsMacOS) block in the finally above?

Sorry, something went wrong.

Uh oh!

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

@thezim Greg Zimmerman (thezim) Nov 3, 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 would agree. Since fpm is in a try, if it fails the next fatal error would be here leaving it in a bad state. I'll go ahead and move it.

As an aside I'm not unsure if the try should even be there for the fpm call. I would think you would want the build point of failure reported at the fpm call and not a dependancy later in the code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@thezim
Move cleanup up closer to fpm call.
7012c62
@TravisEz13

Travis Plunk (TravisEz13) commented Nov 4, 2017

Copy link
Copy Markdown
Member

Packaging build passed here: https://travis-ci.org/thezim/PowerShell/jobs/297030200
All reactions

Sorry, something went wrong.

Uh oh!

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

Travis Plunk (TravisEz13) commented Nov 4, 2017

Copy link
Copy Markdown
Member

@PowerShell/powershell-maintainers Joey Aiello (@joeyaiello) Steve Lee (@SteveL-MSFT) Since this previously regressed after working, I would consider this at least moderately risky. Do we want to take this for 6.0.0?
I think we should. I believe we have addressed the issues (thanks Greg Zimmerman (@thezim)) and we shipped the last beta with this feature.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thezim Greg Zimmerman (thezim) mentioned this pull request Nov 4, 2017
add Uniform Type Identifier to osxpkg #5323
Merged
@thezim

Greg Zimmerman (thezim) commented Nov 4, 2017

Copy link
Copy Markdown
Contributor Author

We should ensure if #5323 merges first that we include the UTI update to CFBundleIdentifier in the plist in this PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13 Travis Plunk (TravisEz13) changed the title Enable macOS launcher [Don't Merge] Enable macOS launcher Nov 5, 2017
@TravisEz13

Travis Plunk (TravisEz13) commented Nov 5, 2017

Copy link
Copy Markdown
Member

#5323 was merged. I updated the title so we don't accidentally merge until the PR is updated.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thezim
Update UTI.
96a7b56
@thezim

Greg Zimmerman (thezim) commented Nov 5, 2017

Copy link
Copy Markdown
Contributor Author

UTI updated. Packaging build passed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13 Travis Plunk (TravisEz13) changed the title [Don't Merge] Enable macOS launcher Enable macOS launcher Nov 6, 2017
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Nov 6, 2017

Copy link
Copy Markdown
Member

Travis Plunk (@TravisEz13) I agree that we should take this fix for 6.0.0, since the beta.9 macOS package already had the macOS launcher change.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Nov 6, 2017
View reviewed changes
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Nov 6, 2017
View reviewed changes

@adityapatwardhan 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.

I agree we should take this change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@TravisEz13
Travis Plunk (TravisEz13) merged commit a674c51 into PowerShell:master Nov 6, 2017
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@thezim @TravisEz13
Enable macOS launcher (PowerShell#5291)
dd96791
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes

Uh oh!

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

Assignees

@adityapatwardhan 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.

4 participants

@thezim @TravisEz13 @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 01402503 — 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.