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 cleanup leaving generated files behind #312

Merged
merged 3 commits into from Dec 1, 2023

Conversation

matthijskooijman
Copy link
Contributor

This causes an issue with the Debian package, where running a build and then a clean should leave the source tree in the same state as it started in (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1045259).

I'm about to ship these patches as part of the 0.7.4 Debian package, but it would be better to fix these upstream.

I'm not 100% sure that hardcoding the filenames to delete in setup.py is ideal, but it is pragmatic and probably not worth the trouble delegating the deletion all the way into parser.py and tokens.py where the original filenames are defined.

This prevents hardcoding the path to this command, so things stay
working if it is ever changed.

Also, this prepares for overriding the clean command next, which would
otherwise require importing the deprecated and to be removed in py3.12
distutils package.
The setup.py file overrides the build step to generate parsetab.py and
lextab.py, but does not clean them up. This adds an override to the
clean step to remove them again.

Note that this uses the distutils version of the command as a basis,
since (unlike the build_py step) setuptools does not provide its own
version of it but just uses the distutils version.
@matthijskooijman
Copy link
Contributor Author

I've updated the PR, adding the first commit to prevent having to import distutils directly (which is deprecated and expected to be removed in an upcoming python version).

@glx22 glx22 merged commit b526402 into OpenTTD:master Dec 1, 2023
21 checks passed
@matthijskooijman
Copy link
Contributor Author

Hm, it seems these changes were merged in 0.7.5, but were not mentioned in docs/changelog.txt. Should I have added a changelog line in my PR? Or was this just overlooked in the release? Maybe good to add it retroactively for future reference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants