Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Save man files in /usr/share/man instead of /usr/local/share/man #23855

Merged
merged 2 commits into from
Aug 19, 2024
Merged

Save man files in /usr/share/man instead of /usr/local/share/man #23855

merged 2 commits into from
Aug 19, 2024

Conversation

rzippo
Copy link
Contributor

@rzippo rzippo commented May 27, 2024 •
edited
Loading

PR Summary

This PR fixes #13517, #16679, #19217, #23835.
The man files should go to /usr/share/man, not /usr/local/share/man.
The latter not only is against packaging policies, it also breaks on OSTree-based systems where the /usr/local/share location is not writable during package install.

The fix was tested by building a 7.4.2 package (see this branch) and installing it on Aurora-DX.

PR Context

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Jun 3, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Jun 3, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Jun 11, 2024
@SteveL-MSFT SteveL-MSFT closed this Jul 1, 2024
@SteveL-MSFT SteveL-MSFT reopened this Jul 1, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Jul 1, 2024
@SteveL-MSFT
Copy link
Member

Forcing rerun of CI

@JamesWTruher
Copy link
Member

@rzippo - I'm concerned about MacOS, wouldn't it be better to make this change for Linux only (especially since brew installs PowerShell in `/usr/local/Cellar')

JamesWTruher
JamesWTruher suggested changes Jul 1, 2024
View reviewed changes
Copy link
Member

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this change should only be for Linux as MacOS brew installs in /usr/local

@microsoft-github-policy-service microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 1, 2024
@andyleejordan
Copy link
Member

andyleejordan commented Jul 1, 2024 •
edited by unfurl-links bot
Loading

I agree with Jim. If our Linux packages are installing everything else to /usr/ not /usr/local, then the same should go for the man page. Which correspondingly means that on macOS where the "package manager" (brew) installs to /usr/local it should remain there...which, sidebar, did we never contribute (or did we attempt and it was refused) a formula to brew? I don't see powershell nor pwsh at https://github.com/Homebrew/homebrew-core/tree/master/Formula/p, and I only found our tap https://github.com/PowerShell/Homebrew-Tap/blob/master/Formula/powershell.rb.

GitHub
🍻 Default formulae for the missing package manager for macOS (or Linux) - Homebrew/homebrew-core
GitHub
Contribute to PowerShell/Homebrew-Tap development by creating an account on GitHub.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 2, 2024
@rzippo
Copy link
Contributor Author

rzippo commented Jul 2, 2024

I agree with this change for MacOS given your arguments, but I don't have a MacOS system to test with.

Judging from the rest of the code, $Environment.IsMacOS and $Environment.IsLinux can be used to distinguish the two cases and assign the correct path to $ManFile, so I added code to do this.
Please check that it works as you expect.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Jul 9, 2024
@SteveL-MSFT SteveL-MSFT added the CommunityDay-Small A small PR that the PS team has identified to prioritize to review label Aug 5, 2024
JamesWTruher
JamesWTruher approved these changes Aug 5, 2024
View reviewed changes
Copy link
Member

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks better

I would be happier to see some validation for this, but that doesn't block this PR.

adityapatwardhan
adityapatwardhan approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@adityapatwardhan adityapatwardhan added CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log and removed Review - Needed The PR is being reviewed labels Aug 19, 2024
@adityapatwardhan adityapatwardhan merged commit 68a18d4 into PowerShell:master Aug 19, 2024
24 checks passed
@microsoft-github-policy-service Microsoft GitHub Policy Service
Copy link
Contributor

microsoft-github-policy-service bot commented Aug 19, 2024 •
edited by unfurl-links bot
Loading

📣 Hey @rzippo, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

@adityapatwardhan
Copy link
Member

@rzippo Thank you for your contribution!

chrisdent-de pushed a commit to chrisdent-de/PowerShell that referenced this pull request Sep 12, 2024
@rzippo @chrisdent-de
Save man files in /usr/share/man instead of /usr/local/share/man (P…
80e3a38 
…owerShell#23855)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesWTruher JamesWTruher JamesWTruher approved these changes

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@Jack-GitHub520 Jack-GitHub520 Jack-GitHub520 approved these changes

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13 TravisEz13 is a code owner

@anmenaga anmenaga Awaiting requested review from anmenaga anmenaga is a code owner

Assignees

@adityapatwardhan adityapatwardhan

Labels
CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log CommunityDay-Small A small PR that the PS team has identified to prioritize to review
Projects
None yet
Milestone
No milestone
6 participants
@rzippo @SteveL-MSFT @JamesWTruher @andyleejordan @adityapatwardhan @Jack-GitHub520