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 guide for development with CLion #5743

Merged
merged 2 commits into from
Apr 11, 2022
Merged

Conversation

badlogic
Copy link
Contributor

@badlogic badlogic commented Apr 6, 2022

This improved guide describes how to use the compilation database generated by SCons to work on Godot with CLion. It also describes a setup that allows single click cleaning/building/debugging/profiling of the Godot editor directly from within CLion. As a heavy CLion user, I can confirm that this should be the preferred workflow when using CLion for Godot development.

@Calinou
Copy link
Member

Calinou commented Apr 6, 2022

One issue that was raised on the Godot contributors chat is that CLion doesn't seem to expand shell command usage such as scons -j$(nproc). This causes an error as $(nproc) will be passed as-is to the SCons command, while it should be converted to a numeric value by the shell.

The documentation should probably have a warning about this, telling the user to replace -j$(nproc), -j%NUMBER_OF_PROCESSORS or -j$(sysctl -n hw.logicalcpu) with a fixed amount of CPU cores like -j8.

To resolve this in the future, I requested the addition of an universal --parallel command line option on the SCons mailing list: https://pairlist4.pair.net/pipermail/scons-users/2022-April/008840.html

@badlogic
Copy link
Contributor Author

badlogic commented Apr 7, 2022

Thank you for the swift review and sorry for the many edits you had to make.

Bill had a very good suggestion over on the SCons mailing list regarding the -j issue: https://pairlist4.pair.net/pipermail/scons-users/2022-April/008841.html

I suppose this could make it into Godot's SConstruct file as a default? I'll add a callout regarding -j to this PR for the current setup.

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

Nice to see you around these parts :) Looks good to me.

I'll wait for your amendment before merging. A squash would be appreciated!
Some of the screenshots seem on the larger side, but if you're busy I can run a compressor on them.

Edit:

Bill had a very good suggestion over on the SCons mailing list regarding the -j issue: https://pairlist4.pair.net/pipermail/scons-users/2022-April/008841.html

I suppose this could make it into Godot's SConstruct file as a default? I'll add a callout regarding -j to this PR for the current setup.

I'll see about this. There may be reasons we don't currently do this, but I'm not aware.

@Calinou
Copy link
Member

Calinou commented Apr 7, 2022

I'll see about this. There may be reasons we don't currently do this, but I'm not aware.

Relevant conversation on the Godot contributors chat in #buildsystem:

<Calinou> it looks like we can add a custom parallel=yes option to SCons to automatically use the number of CPU threads on the system: https://pairlist4.pair.net/pipermail/scons-users/2022-April/008841.html
pairlist4.pair.net
<Calinou> the reason for doing this is that it will work well in IDEs that don't expand shell commands or variables, and it avoids having to remember one-liners for each platform 🙂
<Calinou> it needs to be optional though, as you can't override the number of threads with -j if you override it in your SCons setup. It could be enabled by default, but this would require users to use parallel=no -j1 to override it

<Akien> It could likely still be overridden, you can check in the code block for if env["parallel"] whether there's a -j X in the args list.
<Akien> Ah no, -j doesn't register in the ARGUMENTS dict.

Apply suggestions from code review

Apply suggestions from code review

Apply suggestions from code review

Add callout about CLion not expanding shell commands with example.
@badlogic
Copy link
Contributor Author

@mhilbrunner @Calinou Sorry for this taking so long. I've now amended the PR as discussed and squashed the commits. Thank you for your review.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Thanks! Congratulations for your first merged pull request 🎉

@Calinou Calinou changed the title Improved guide for development with CLion. Improve guide for development with CLion Apr 11, 2022
@Calinou Calinou merged commit e2a22b0 into godotengine:master Apr 11, 2022
rsubtil pushed a commit to rsubtil/godot-docs that referenced this pull request Apr 11, 2023
Co-authored-by: Hugo Locurcio <hugo.locurcio@hugo.pro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants