Enable macOS launcher #5291
Enable macOS launcher #5291
Conversation
thezim
requested review from
TravisEz13,
adityapatwardhan and
daxian-dbw
as code owners
|
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. |
|
@TravisEz13 Done. |
|
Verified packaged correctly here: https://travis-ci.org/thezim/PowerShell/builds/296027725 |
tools/packaging/packaging.psm1
Outdated
|
|
||
| # Set permissions. | ||
| Start-NativeExecution { | ||
| find $macosapp | xargs chmod 755 |
There was a problem hiding this comment.
Can you undo this when it's finished? Or remove the folder when it's done?
There was a problem hiding this comment.
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.
|
packaging built correctly here: https://travis-ci.org/thezim/PowerShell/jobs/296567451 |
|
relaunched AppVeyor due to myget failure.. |
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")) |
There was a problem hiding this comment.
Should this cleanup code be moved to the if ($Environment.IsMacOS) block in the finally above?
There was a problem hiding this comment.
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.
|
Packaging build passed here: https://travis-ci.org/thezim/PowerShell/jobs/297030200 |
|
@PowerShell/powershell-maintainers @joeyaiello @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? |
|
We should ensure if #5323 merges first that we include the UTI update to |
|
#5323 was merged. I updated the title so we don't accidentally merge until the PR is updated. |
|
UTI updated. Packaging build passed. |
|
@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. |
adityapatwardhan
left a comment
adityapatwardhan
left a comment
There was a problem hiding this comment.
I agree we should take this change.
4 participants
Notable change this time around.
Info.plistby default to prevent conflicts if a fresh clone.CFBundleIdentifieris 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.plutilis enough to make this happen.Fixes #5262