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

Fix compiledb SCons tool availability #89481

Merged

Conversation

adamscott
Copy link
Member

@adamscott adamscott commented Mar 14, 2024

Currently, scons compiledb compiledb=no fails to run, as the compiledb alias isn't set if the compiledb variable isn't True.

X@X godot $ scons -Q compiledb
Automatically detected platform: linuxbsd
Auto-detected 8 CPU cores available for build parallelism. Using 7 cores by default. You can override it with the -j argument.
Note: Using `execinfo=yes` for the crash handler as required on platforms where glibc is missing.
Building for platform "linuxbsd", architecture "x86_64", target "editor".
[Initial build] scons: *** Do not know how to make File target `compiledb' (/home/X/srg/godot/compiledb).  Stop.
[Time elapsed: 00:00:03.429]

This PR makes the compiledb tool available for being summoned directly.

The variable is only used to indicate that we want to run the compiledb tool on the main build.

SConstruct Outdated
Comment on lines 17 to 21
# SCons
from SCons import __version__ as scons_raw_version

# SCons version
scons_ver = Environment._get_major_minor_revision(scons_raw_version)
Copy link
Member

Choose a reason for hiding this comment

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

I assume black isn't happy with keeping this included just before it's used?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's just that I thought that it could be a nice global variable. I can set it as it was, if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I prefer to keep it close to where it's used. It's a hack, using a private API, so it's not something we should encourage to rely on.

We could even drop it and increase the min SCons version to 4.0.0, but that would remove support for Ubuntu 20.04 LTS, which is still commonly used to make custom builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't we build install scons using pip on Ubuntu 20.04 LTS?

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, I reverted back these lines.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we build install scons using pip on Ubuntu 20.04 LTS?

We don't build on Ubuntu 20.04 LTS, but many users do. And yes they could install SCons with pip, but this adds some friction, and is not a suitable option everywhere (e.g. for distro packaging, though I doubt Ubuntu will start packaging Godot for Ubuntu 20.04).

@adamscott adamscott force-pushed the fix-compiledb-tool-availability branch from aa62116 to 6e24980 Compare March 14, 2024 14:22
SConstruct Outdated Show resolved Hide resolved
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 14, 2024
Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

Looks fine!

@adamscott adamscott force-pushed the fix-compiledb-tool-availability branch from 6e24980 to 4080992 Compare March 14, 2024 15:59
@adamscott
Copy link
Member Author

@akien-mga @Riteo Fixed up the superfluous // SCons. But I added/reordered comments to make it clearer where features begin and end.

@akien-mga akien-mga merged commit 2c7c777 into godotengine:master Mar 14, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

3 participants