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

Print build info used when using godot --build-info #40627

Closed
wants to merge 1 commit into from

Conversation

Anutrix
Copy link
Contributor

@Anutrix Anutrix commented Jul 23, 2020

Print build arguments used when using godot --version.
I couldn't find how official builds are generated without hashes so that part is still not done.

Bugsquad edit: This partially addresses #28617.

@Anutrix
Copy link
Contributor Author

Anutrix commented Jul 23, 2020

Force-pushed after using black -l 120 SConstruct to format SConstruct.
This doesn't seem to be documented anywhere in the docs yet.

@YuriSizov
Copy link
Contributor

Force-pushed after using black -l 120 SConstruct to format SConstruct.
This doesn't seem to be documented anywhere in the docs yet.

Well, code style contributing guide mentions using pre-commit hooks, but doesn’t list the new ones explicitly.

https://docs.godotengine.org/en/stable/community/contributing/code_style_guidelines.html#pre-commit-hook

@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:core labels Jul 23, 2020
@Calinou Calinou added this to the 4.0 milestone Jul 23, 2020
@Calinou
Copy link
Member

Calinou commented Jul 23, 2020

Note that this won't list arguments that were specified in custom.py and in the SCONS_FLAGS environment variable. For SCONS_FLAGS, we can prepend its contents to the sys.argv[1:] line. As for custom.py, it's more difficult as it's a full-fledged Python script.

Also, some arguments such as progress=no or -j8 are probably not worth mentioning in the list of build flags. Still, I think the current implementation in this PR is better than nothing, so it's not blocking either.

@Anutrix
Copy link
Contributor Author

Anutrix commented Jul 23, 2020

Another way to do this was by using SCons's ARGUMENTS dictionary but that doesn't include arguments like -j5. Not sure if it includes flags from custom.py but my guess is that it doesn't.

SConstruct Outdated Show resolved Hide resolved
@Xrayez
Copy link
Contributor

Xrayez commented Jul 23, 2020

Note that you can look at .scons_env.json to have an overview of what kind of parameters were used during a build (build-time over run-time), just to eliminate XY problems for some people.

@Anutrix
Copy link
Contributor Author

Anutrix commented Jul 23, 2020

.scons_env.json seems to have too much data which is why I didn't touch it.

@Xrayez
Copy link
Contributor

Xrayez commented Jul 23, 2020

I believe you could also add some essential build arguments to this first, so they cannot go ignored (many have just defaults), and append the rest of the build arguments from any other source, including sys.argv (you might need to make sure that those arguments are unique).

@akien-mga
Copy link
Member

akien-mga commented Jul 23, 2020

IMO just hardcoding builds args is not so useful per se. I'd prefer an approach that gives the value of some key build options like tools, target, lto, etc. (and then additionally and line args, why not, but we indeed don't care about args that only impact the build host like -j4 or verbose - what matters is what makes the binary different from another one). I'd put this behind a --build-info flag.

@Anutrix
Copy link
Contributor Author

Anutrix commented Jul 23, 2020

1st option:
sys.argv[1:]

2nd option:
We can use SCons's ARGUMENTS dictionary which doesn't include -j3 argument.

3rd option:
.scons_env.json has all the build flags. Even if we just consider the exposed commands we get L397-L505. These 109 flags include 55 module_*_enabled flags, 24 builtin_* flags and 30 other flags among which ANDROID_NDK_ROOT is one flag that can be safely ignored.
Assuming we include everything above, we can reduce it to 31(30-1+One super long line with enabled modules+One long line with builtins used) lines.

We would need to manually remove progress, verbose and any empty arguments.
Additionally, irrespective of any of these options chosen, we can remove any flags that have the default values from the result but that would make their output pretty much the same.

Not sure which option would be best.

@aaronfranke
Copy link
Member

@Anutrix Is this still desired? If so, it needs to be rebased and tested on the latest master branch.

@Anutrix
Copy link
Contributor Author

Anutrix commented Feb 28, 2021

I'm not sure if this is still needed. If I get a response in some time I'll rebase it. We can close it if noone's interested.

@YuriSizov YuriSizov requested a review from a team August 24, 2021 23:26
@Anutrix Anutrix requested a review from a team as a code owner October 17, 2021 16:46
@Anutrix Anutrix force-pushed the scons-build-arg branch 2 times, most recently from ddc3e42 to 6986d05 Compare October 17, 2021 19:06
@Anutrix Anutrix changed the title Print build arguments used when using godot --version Print build info used when using godot --build-info Oct 17, 2021
@Anutrix
Copy link
Contributor Author

Anutrix commented Oct 17, 2021

Rebased and tested on master.
It shows the build args and SCONSFLAGS environment variable.

It can be improved as it doesn't support custom.py or even list out default values but it should be helpful till those are implemented in the future.

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.

The --build-info command line argument should be listed in the command line help (main/main.cpp). It should also be listed in the Bash, zsh and fish shell completion files (misc/dist/shell/). See #59153 Files changed tab for an example of doing this.

@Calinou
Copy link
Member

Calinou commented Mar 18, 2022

I tested this PR rebased on master, it seems to work:

❯ bin/godot.linuxbsd.tools.64.llvm --build-info
4.0.alpha.custom_build.bd77c981a
Build arguments: platform=linuxbsd use_llvm=yes LINKFLAGS=-fuse-ld=mold -j10 tests=no

It might be worth looking into stripping the -j command line argument (and the number immediately after it), so that it's not included in the BUILD_ARGUMENTS constant as it's not relevant. This is not critical for an initial implementation still, as this can still be useful for troubleshooting (and distinguishing your own custom builds, similar to #59247).

@akien-mga
Copy link
Member

akien-mga commented Mar 18, 2022

I still think this is a bad idea with the current implementation. It needs to expose the actual configuration used, not which command line parameters were passed:

IMO just hardcoding builds args is not so useful per se. I'd prefer an approach that gives the value of some key build options like tools, target, lto, etc. (and then additionally and line args, why not, but we indeed don't care about args that only impact the build host like -j4 or verbose - what matters is what makes the binary different from another one).

@Anutrix
Copy link
Contributor Author

Anutrix commented May 21, 2022

Closing it so that someone can work on better and cleaner version.

(Maybe someone/new contributors can use this as a simple reference.)

@Anutrix Anutrix closed this May 21, 2022
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels May 21, 2022
@Anutrix Anutrix deleted the scons-build-arg branch July 26, 2022 02:11
@AThousandShips AThousandShips removed this from the 4.0 milestone Jan 11, 2024
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.

7 participants