-
Notifications
You must be signed in to change notification settings - Fork 694
Reorganize platform build instructions #4110
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
Conversation
Note for reviewers: you can preview the readme on GH by going to https://github.com/canonical/multipass/tree/platform-build-instructions?tab=readme-ov-file#contributing and you can click on the individual build files for their own renderings. |
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.
Pull Request Overview
This PR refactors and clarifies the project’s build instructions by moving platform-specific steps into dedicated documents and reflowing the README.
- Reformats README introduction, project status table, and usage pointers
- Introduces separate BUILD.linux.md, BUILD.macOS.md, and BUILD.windows.md for platform-specific build steps
- Updates Windows and macOS build guides with improved wrapping and notes
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
README.md | Reflowed intro text, reformatted table, and updated build section to reference platform docs |
BUILD.windows.md | Wrapped long lines, clarified Chocolatey and Defender steps, and reorganized dependency install |
BUILD.macOS.md | Wrapped sections, separated Qt install options, and clarified Homebrew/CMake steps |
BUILD.linux.md | Added a new Linux build guide with Environment Setup, build, and run instructions |
Comments suppressed due to low confidence (3)
BUILD.windows.md:11
- The executable name casing is incorrect. It should be
PowerShell.exe
(capital ‘S’).
Press Windows Key+X and Run Windows PowerShell(Admin) then follow the chocolatey instructions to "Install with Powershell.exe"
BUILD.windows.md:19
- There's a typo: "thread protection settings" should be "threat protection settings".
Windows Defender Security Center, go to Virus & threat protection, then Virus and thread protection settings, disable Real-time protection.
BUILD.macOS.md:76
- The command
opem config env
appears to be a typo or unclear; please confirm and correct the intended command.
Then add the entries to the $PATH as above, and add the variables reported by `opem config env`.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4110 +/- ##
=======================================
Coverage 89.31% 89.31%
=======================================
Files 259 259
Lines 15715 15715
=======================================
Hits 14036 14036
Misses 1679 1679 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Just some small updates to the build instructions. Let me know how important you think they are.
Install the latest stable version of Qt6 LTS (6.2.4 at the moment): <http://www.qt.io/download-open-source/>. | ||
|
||
If it tells you that XCode 5.0.0 needs to be installed, go to XCode > Preferences > Locations and make a selection in the _Command Line Tools_ box. | ||
If it tells you that XCode 5.0.0 needs to be installed, go to XCode > Preferences > Locations and make a selection in | ||
the _Command Line Tools_ box. | ||
|
||
Add Qt6 to your PATH environment variable, adding to your `.bash_profile` file the following line: | ||
|
||
export PATH=$PATH:~/Qt/6.2.4/clang_64/bin |
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.
This might be different based on architecture. On my ARM Mac, it's at ~/Qt/6.2.4/macos/bin
@@ -45,7 +56,8 @@ Alternatively if using Qt6 from Homebrew, do | |||
|
|||
cmake -Bbuild -H. -GNinja -DCMAKE_PREFIX_PATH=/usr/local/opt/qt6 |
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 this should be symmetrical with the previous statement on setting Qt environment variables.
@@ -45,7 +56,8 @@ Alternatively if using Qt6 from Homebrew, do | |||
|
|||
cmake -Bbuild -H. -GNinja -DCMAKE_PREFIX_PATH=/usr/local/opt/qt6 | |||
|
|||
or, if on Apple silicon, brew will store the Qt binaries in a different location. Additionally, OpenSSL will be in a similar location; `/opt/homebrew/Cellar/openssl@3`, which can be set in the project level `CMakeLists.txt` file. | |||
or, if on Apple silicon, brew will store the Qt binaries in a different location. Additionally, OpenSSL will be in a |
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 remove mentions of installing Qt via brew, since brew does not carry the correct version.
@@ -54,11 +66,15 @@ Then start the build with: | |||
cd build/ | |||
ninja | |||
|
|||
Take care to adjust the `CMAKE_PREFIX_PATH` to the location you installed Qt above, or else cmake will complain about missing Qt6. | |||
Take care to adjust the `CMAKE_PREFIX_PATH` to the location you installed Qt above, or else cmake will complain about | |||
missing Qt6. | |||
|
|||
Building in QtCreator |
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.
Do we want to "maintain" building Multipass in QtCreator?
|
||
### Qt6 | ||
|
||
Install the latest stable version of Qt6 (6.2.4 at the moment): <https://www.qt.io/download-thank-you?os=windows/>. |
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.
6.2.4
is no longer the "latest" stable version.
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.
IMO, we can just get rid of the version information, unless we want to pin QT to a specific stable version.
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.
Either that or "latest stable version". But this is kind of outside scope here, I just want to have all platforms linked from the Readme... I'll open an issue for improvements specifically.
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.
6.2.4
is a specific version that we align with. It's just that there newer LTS versions, so it's not exactly the "latest" anymore.
@@ -94,24 +105,28 @@ Running multipass | |||
--------------------------------------- | |||
|
|||
### Enable Hyper-V |
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.
Not strictly required 🤷
Hey @sharder996, I agree the instructions need fixing in a bunch of places, but that was not the intention of this PR. I guess the title is confusing, I'll update it. This was only meant to fix #4078 (to complete the merger essentially). Apart from that, I just reformatted the markdown (to be consistent with the new file) and renamed a file. |
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.
Approving based on the assumption that currently outlined issues in the build instructions will be fixed in a later PR.
BUILD.linux.md
Outdated
this case, you need to manually fetch the tags from the upstream by running | ||
`git fetch --tags https://github.com/canonical/multipass.git` in the `<multipass>` source code directory. | ||
|
||
### Automatic linker selection |
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.
FWIW, this applies to all platforms, not just Linux
.
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.
Good catch!
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.
Alright, I moved this section to the central readme and adjusted a little. Let me know what you think.
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.
LGTM, except ### Automatic linker selection
Reorganize build instructions, to consistently cover all platforms that are now targeted by the single repository. To that end, link to platform-specific build instructions for each platform from the root README.md, extracting Linux instructions into its own file.
2392a60
to
35f7e43
Compare
@sharder996, sorry if I sounded dismissive. I understand and share your concerns regarding the contents of these build files, it's just that I would rather not get attached to fixing them myself at this point. Doing so properly requires verifying in clean environments and needs a little bit of time (note that the only reason that stuff figures in this PR at all is that I hit Ctrl+Alt+L in CLion to reformat, then tweaked a few newlines). I am perfectly open to scheduling this for an upcoming pulse though. |
@ricab Yup, no problem. I was thinking the same thing when reviewing and it was just an opportunity to note all the problems with the instructions. 😄 |
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.
LGTM.
Reorganize platform build instructions
Fix #4078. See commit descriptions for more information.
MULTI-1953