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

Improve Cross-Platform Build Instructions in Documentation #3229

Merged
merged 3 commits into from
Oct 27, 2024

Conversation

Mq-b
Copy link
Contributor

@Mq-b Mq-b commented Oct 27, 2024

Description:

This PR updates the build instructions in the documentation to make them more cross-platform and compatible with different development environments, such as Windows with MSVC.

Details:

Previously, the documentation included commands specific to GNU-based toolchains, notably make. However, this can be problematic for users on non-GNU systems (such as Windows with MSVC), where make is not available by default. This update replaces make -j with the more universally supported cmake --build . --parallel and adds a clear cmake --install . command for the installation step.

Reasoning:

  • Cross-Platform Compatibility: The updated commands are compatible across platforms, as CMake will automatically choose the appropriate build system (e.g., MSBuild on Windows, make on Unix-like systems).
  • Improved Readability: By breaking down the commands into separate lines, the process becomes easier to follow, especially for users who are not familiar with the default GNU environment assumptions.
  • Consistent CMake Usage: Using cmake --build and cmake --install promotes consistency with CMake’s intended workflows and is less reliant on external toolchains.

Thank you for considering this contribution to enhance the usability of the build instructions for a broader range of developers.

@gabime
Copy link
Owner

gabime commented Oct 27, 2024

Thanks. Could you make it shorter? It takes now 7 lines

@Mq-b
Copy link
Contributor Author

Mq-b commented Oct 27, 2024

Thanks. Could you make it shorter? It takes now 7 lines

Thank you for your reply. Okay, I'll do that.

@Mq-b
Copy link
Contributor Author

Mq-b commented Oct 27, 2024

I compiled the tests in Ubuntu22.04 using gcc11 and windows 11 using the Developer PowerShell for VS 2022 terminal with no problems.

@Mq-b
Copy link
Contributor Author

Mq-b commented Oct 27, 2024

@gabime

@gabime
Copy link
Owner

gabime commented Oct 27, 2024

using the original && style is better. All is needed is replacing a single line: the “make build -j” line with cmake —build .”

@Mq-b
Copy link
Contributor Author

Mq-b commented Oct 27, 2024

using the original && style is better. All is needed is replacing a single line: the “make build -j” line with cmake —build .”

The problem is that windows powershell terminals don't support this!

@Mq-b
Copy link
Contributor Author

Mq-b commented Oct 27, 2024

using the original && style is better. All is needed is replacing a single line: the “make build -j”

Of course, if you like, of course.

@SainoNamkho
Copy link

SainoNamkho commented Oct 27, 2024

using the original && style is better. All is needed is replacing a single line: the “make build -j” line with cmake —build .”

The problem is that windows powershell terminals don't support this!

Maybe the logical difference counts here. Users may stop copying-pasting commands when one of mutiple lines failed, but with ; one cannot stop.

In addtion, ; breaks cmd meanwhile. I personally suggest that a separate powershell version may be honored.

@gabime gabime merged commit ee16895 into gabime:v1.x Oct 27, 2024
8 checks passed
@gabime
Copy link
Owner

gabime commented Oct 27, 2024

Merged. Thanks @Mq-b

gabime pushed a commit that referenced this pull request Nov 25, 2024
* Update build

* Simplified build command length for cross-platform compatibility.

* Modified to replace `make -j` only with `cmake --build.`
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