Add option to disable git hash

GravisZro commented 1 month ago

Pull Request Type

[ ] GitHub Workflow changes
[ ] Documentation or Wiki changes
[x] Build and Dependency changes
[ ] Runtime changes
- [ ] Render changes
- [ ] Audio changes
- [ ] Input changes
- [ ] Network changes
- [ ] Other changes

Description

Sometimes I need to make builds in deterministic fashion which has made the mandatory git hash being added quite annoying. I was forced to restructure how the d3_version.h file was made. It's much simpler now: default values unless overridden by compiler defines. The whole thing was overly complex to start with.

The new option is called ENABLE_GITHASH and it's a boolean value that defaults to ON.

Related Issues

Screenshots (if applicable)

Checklist

[x] I have tested my changes locally and verified that they work as intended.
[x] I have documented any new or modified functionality.
[x] I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
[x] I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Additional Comments

winterheart commented 1 month ago

We added git hashing for easy identification and diagnostic. There no additional issues that every commit forces incremental build of changed parts of project. You just gets one little step on generating additional file.

Sorry, but knowing on what commit user gets troubles is more useful. You also removed project based version generation, and I don't see any reason why we shouldn't use this feature.

Lgt2x commented 1 month ago

We added git hashing for easy identification and diagnostic. There no additional issues that every commit forces incremental build of changed parts of project. You just gets one little step on generating additional file.

Sorry, but knowing on what commit user gets troubles is more useful. You also removed project based version generation, and I don't see any reason why we shouldn't use this feature.

As mentioned in this comment, issues can arise when the git information is stripped from the code. I think the intention here is to address this case

winterheart commented 1 month ago

We added git hashing for easy identification and diagnostic. There no additional issues that every commit forces incremental build of changed parts of project. You just gets one little step on generating additional file. Sorry, but knowing on what commit user gets troubles is more useful. You also removed project based version generation, and I don't see any reason why we shouldn't use this feature.

As mentioned in this comment, issues can arise when the git information is stripped from the code. I think the intention here is to address this case

This case already covered by pregenerated git-hash.txt file. On packaging step (which is not implemented yet) this file will be included to source tarball, and build system will know on what commit tarball was generated. Even if it fails (like no git-hash.txt at all) we still covered that case - GIT_HASH will be empty string.

bryanperris commented 1 month ago

Is this something that bothers you for when its in the binary filename or when its displayed in the game?

GravisZro commented 1 month ago

Ok, this is another case of nobody reading the actual PR.

winterheart

Sorry, but knowing on what commit user gets troubles is more useful. You also removed project based version generation, and I don't see any reason why we shouldn't use this feature.

@winterheart I know that which is why it's ENABLED BY DEFAULT and can only be disabled by a cmake option. Read what I wrote, dammit because nobody is removing the feature. You closed this PR waaaaay too fast because you have jumped to conclusions. READ THE POST.

Sometimes I need to make builds in deterministic fashion which has made the mandatory git hash being added quite annoying. I was forced to restructure how the d3_version.h file was made. It's much simpler now: default values unless overridden by compiler defines.

bryanperris

I need to be able to make a deterministic build. This means I need to be able to compare git commit with another git commit to ensure the same binary has been generated. I cannot do that if the git hash is part of the binary.

bryanperris commented 1 month ago

You don't like the fact that the hash string keeps altering the binary's hash checksum?

winterheart commented 1 month ago

Ok, this is another case of nobody reading the actual PR.

winterheart

Sorry, but knowing on what commit user gets troubles is more useful. You also removed project based version generation, and I don't see any reason why we shouldn't use this feature.

@winterheart I know that which is why it's ENABLED BY DEFAULT and can only be disabled by a cmake option. Read what I wrote, dammit because nobody is removing the feature. You closed this PR waaaaay too fast because you have jumped to conclusions. READ THE POST.

I do not agree with your approach "It's annoying me, I cannot understand logic behind it and I want it be removed". With your rational on disabling git_hash (which nobody else complained) you entirely removing version generation based on project property without any cause for it. Instead of that you added add_compile_definitions and now on EVERY commit WHOLE project will be recompiled just because global D3_GIT_HASH define was changed. This should be avoided in order speed up building.

So please, be polite, be nice and get ready that some of your contributions can be rejected.

bryanperris commented 1 month ago

I do like having the git hash burned into the binary like how vehicles have a unique vin number etched into the frame. That way each build is stamped and are unique and also the dirty bit can help identify someone making a build with uncommitted changes or they do commit, but the hash doesn't exist in this project's changelog history.

Always been inspired from this source here: https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/Common/scmrev.h.in

JeodC commented 1 month ago

I don't think the motivation here is removal of an "annoying" feature. However, winterheart is correct that in effort to allow deterministic builds, this pull request converted GIT_HASH to a global compile definition, which means that every time the hash changes, every file in the source gets recompiled. The goal is better accomplished, I think, by selectively including GIT_HASH only in files where it's needed, which is the main menu code--could even be done with a header file.

That could have been pointed out in a review or comment rather than a dismissive closure, though.

GravisZro commented 1 month ago

@JeodC I agree which part of why #393 exists.

DescentDevelopers / Descent3