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.
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.
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
Update installation README #131
Update installation README #131
Changes from 4 commits
7a43b9b
2a8ca85
290254a
84a25ed
e1d4989
13828a4
59ee9d5
771f270
3962356
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 definitely shouldn't need admin permissions, and using them is more likely to introduce problems (if you've needed admin permissions, it's likely because you've previously run something with admin permissions you weren't supposed to, and that's meant a file or directory was created with admin ownership). If you're not running software that was originally written for Windows XP or earlier, running things as admin absolutely shouldn't be in your toolbox as a troubleshooting step. If you're hitting problems, the software's got a bug (or you're using it incorrectly), and giving potentially buggy software greater powers to ruin your system is like giving a toddler an uzi because they're shooting a nerf gun at things they're not supposed to - it's dangerous and there's no logical reason to expect it'd help.
I also don't particularly like the swap between a regular shell being the default and the MSVC-activated one being the default. I'm confident it's a minority of people who use the Microsoft repacks of Git for Windows and CMake that are provided by the Visual Studio installer instead of the official distributions, and it's only those people who benefit from the x64 Native Tools Command Prompt (or any of its siblings) as the regular Git and CMake installers put them on the PATH, and if you remove them, using the MSVC shell won't put them back.
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 second this, there are no reasons to either use an administrator prompt, and using the VS command prompt just to get the shipped CMake / Git is a bit overkill.
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.
Visual Studio doesn't install cmake to the windows PATH when selected in the VS installer, which causes mob to outright fail when the VS x64 native tools cmd isn't used, unless CMake is installed outside of the VS installer.
I've assumed Git is the same, but I could be wrong. I agree we should be advising the use of the modern terminal, but that means re-writing a large part of the readme to switch to advising using package managers like
winget
orchoco
instead of using the VS installer to install the needed/suggested tools.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.
Updated with some clarifications
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.
There is no suggestion to rely on VS for Git or CMake (except for the optional stuff) in the README. We should add git and CMake as dependencies before Qt or anything.
We do not have to advise installing tools with
winget
,choco
, custom installer, or whatever - Just listing the required tools is enough (that also applies for myaqt
comment). If someone does not manage to installgit
orcmake
when told to, I think they're going to face some issues trying to work on MO2.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 don't really see a difference between either statements, regardless I've pushed another update to appease.
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 don't like adding instructions for a specific package manager because it tends to make it look like you need that particular package manager, which is clearly not the case. And adding instructions for every relevant package managers out there (+ manual installation) is not something we want.
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 think it's really a matter of how it's phrased. It's perfectly possible to do it without making the package manager(s) seem like a requirement or going overboard with detail.
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 true issue would be how we are going to agree on which package manager(s) should be present. 🤡
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.
Chocolatey's the most widely used, so is a shoo-in. Scoop is less popular and generally has a less complete library, so can be skipped. WinGet has Microsoft's backing, so might be worth including. Everything else is so niche that no one will care if it's excluded.