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.

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

Merged
merged 9 commits into from
May 25, 2024
Merged

Conversation

Twinki14
Copy link
Contributor

@Twinki14 Twinki14 commented May 15, 2024

Motivations

The installation section could use some touchups

Modifications

  • Split up the Qt and VS installation parts of the readme
  • Add a new section for Qt that uses aqt to install Qt

Twinki14 added 4 commits May 14, 2024 20:01
# Motivations
The installation section could use some touchups

# Modifications
- Split up the Qt and VS installation parts of the readme
- Add a new section for Qt that uses aqt to install Qt
-
@Twinki14 Twinki14 marked this pull request as ready for review May 15, 2024 00:07
@Holt59
Copy link
Member

Holt59 commented May 15, 2024

Thanks for the updated README, a few comments:

  • MO2 is built against Qt 6.5.0 currently, so the 6.5.3 is off. Might be a good idea to specify a generic version, e.g. QT_VERSION and then put a link to mob.ini for the right version (https://github.com/ModOrganizer2/mob/blob/master/mob.ini#L116).
  • For aqt, I don't think you need Python if you install the exe (either from releases or using a package manager, e.g., winget).

readme.md Outdated
```

## Setting up MOB
- Start the _x64 Native Tools Command Prompt for VS 2022, as Administrator, we need this for cmake & git (not needed if you have either available in your PATH)
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

@Twinki14 Twinki14 May 15, 2024

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 or choco instead of using the VS installer to install the needed/suggested tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with some clarifications

Copy link
Member

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 my aqt comment). If someone does not manage to install git or cmake when told to, I think they're going to face some issues trying to work on MO2.

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 my aqt comment). If someone does not manage to install git or cmake when told to, I think they're going to face some issues trying to work on MO2.

When I'm writing this kind of guide, I do tend to list the names of chocolatey packages for any dependency I mention. While I expect people following the guide to be able to install these tools, it's much less hassle to copy and paste a package name than to search for a package and check it's the same tool as the guide wants.

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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.

readme.md Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
@Twinki14
Copy link
Contributor Author

  • For aqt, I don't think you need Python if you install the exe (either from releases or using a package manager, e.g., winget).

I've tried to steer clear from using package managers as the whole installation part of the readme could be revised to use package managers instead of relying on the VS installer (CMake for example), to keep things consistent I've avoided suggesting package managers

@Twinki14 Twinki14 requested review from Holt59 and AnyOldName3 May 15, 2024 16:16
@Holt59
Copy link
Member

Holt59 commented May 25, 2024

Merging this, we can always update it later if needed.

@Holt59 Holt59 merged commit 11dafd0 into ModOrganizer2:master May 25, 2024
2 checks passed
@Twinki14 Twinki14 deleted the update-readme branch May 25, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants