Add and use version.py
#79671
Add and use version.py
#79671
Conversation
Meant to replace the many ways of doing it
Delete unneded scripts
github-actions
bot
added
<Documentation>
github-actions
bot
added
BasicBuildPassed
|
I haven't looked at the changes remotely in depth yet, but If python is a hard requirement, then this would need a documentation update.
I found it rather convenient to employ github's CI to test platform-sweeping changes like this one. Just make a PR to your own fork, and ensure that all the checks pass (see this mess: https://github.com/moxian/Cataclysm-DDA/pulls). Personal-fork checks would be much faster than upstream too since they all can run in parallel. |
github-actions
bot
added
the
BasicBuildPassed
| cat >VERSION.txt <<EOL | ||
| build type: ${{ matrix.artifact }} | ||
| build number: ${{ needs.release.outputs.timestamp }} | ||
| commit sha: ${{ github.sha }} | ||
| commit url: https://github.com/${{ github.repository }}/commit/${{ github.sha }} | ||
| EOL | ||
| python3 build-scripts/version.py ARTIFACT=${{ matrix.artifact }} TIMESTAMP=${{ needs.release.outputs.timestamp }} |
There was a problem hiding this comment.
Maybe leave this one be? It's not doing any sort of git invocations, or any complex logic.
It's only needed for release, not in the normal dev workflow, there should be no need to move it into python, right? It would make the script more lightweight too.
There was a problem hiding this comment.
True, but I would prefer keeping the option that a release build can be done offline without GHA.
Use logging directly Use locals() instead of globals()
Happens when working in WSL
| @@ -230,7 +230,7 @@ jobs: | |||
| if: ${{ env.SKIP == 'false' && runner.os == 'Linux' }} | |||
| run: | | |||
| sudo apt-get update | |||
| sudo apt-get install libncursesw5-dev ccache gettext parallel | |||
| sudo apt-get install libncursesw5-dev ccache gettext parallel python-is-python3 | |||
There was a problem hiding this comment.
This works for CI, but I'm not convinced we can require our users to have this installed. Even if it works for ubuntu, it might not for other distros.
I think it would be much better to handle this in Makefile instead, since we can. Something like
PYTHON=$(shell which python || which python3)
version:
$(PYTHON) build-scripts/version.py| printf '// NOLINT(cata-header-guard)\n#define VERSION "%s"\n' "$$VERSION_STRING" | tee $(SRC_DIR)/version.h ; \ | ||
| fi \ | ||
| ) | ||
| python build-scripts/version.py VERSION=$(VERSION) ARTIFACT=$(ARTIFACT) |
There was a problem hiding this comment.
ARTIFACT is not defined here at all
| VERSION=<version> | ||
| VERSION_STRING=<version> |
There was a problem hiding this comment.
I think it's surprising to accept both VERSION and VERSION_STRING which are synonyms, and which get overwritten by the output of git anyway.
| try: | ||
| git = subprocess.run(('git', 'rev-parse', '--is-inside-work-tree'), | ||
| capture_output=True) | ||
| except FileNotFoundError: # `git` command is missing |
There was a problem hiding this comment.
The present makefile tolerates lack of git. It might be something worth preserving.
I also realized that it should be technically possible to build cdda from a tarball that has no .git/ in it (and I believe existing Makefile tries to work around that as well)
Summary
Build "Add and use
version.py"Purpose of change
The project was creating
src/version.handVERSION.txtin many ways.Describe the solution
All logic is now handled by a single Python v3.8 script.
This Python version is available in the oldest project baseline.
Windows build was not requiring Python but this PR documentation will add it as requirement.
Describe alternatives you've considered
None
Testing
The following build methods succeed and the game + VERSION.txt show the correct version:
Additional context
Draft PR to let others review while testing.