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

gh-101047: Remove LIBTOOL_CRUFT #101048

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

indygreg
Copy link
Contributor

@indygreg indygreg commented Jan 14, 2023

This variable has been unused since commit
1919983 in 2011.

I noticed it because the arch logic breaks when cross-compiling on macOS.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Please, run make regen-configure to fix the CI :)

@indygreg
Copy link
Contributor Author

I saw the configure failure and was kinda hoping some automation would suggest a fix since I didn't feel like shaving the yak of installing autoconf 2.69. But now that I'm enlightened about the existence of a make rule that pulls a container image with autoconf 2.69, there's no yak to shave. Nice!

Force pushed with the configure regen.

This variable has been unused since commit
1919983 in 2011.

I noticed it because the `arch` logic breaks when cross-compiling on
macOS.

skip news since effectively dead code.
Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It looks OK to me. But, if we're going to clean up vestigial libtool code from the early days of MacOS X, we should also remove the few references to OTHER_LIBTOOL_OPT in configure.ac, configure, and Makefile.pre.in.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@indygreg
Copy link
Contributor Author

Thanks for the PR. It looks OK to me. But, if we're going to clean up vestigial libtool code from the early days of MacOS X, we should also remove the few references to OTHER_LIBTOOL_OPT in configure.ac, configure, and Makefile.pre.in.

I agree with you on the desire for some ancient macOS support code to get purged. But one of the things I learned from being a maintainer of Firefox's build system for several years (and submitting + reviewing thousands of commits to the build system) is that scope bloating build system changes is a recipe for unexpected regressions. Due to the nuance and sensitivity of build systems, I've learned it is best to keep changes small and tightly scoped.

So I'm gently pushing back on the request to scope bloat an otherwise looks OK PR.

(FWIW I'm sitting on a pile of patches to CPython's build system that I'd love to get upstreamed. Many of them involve Apple OS support code. I could potentially add OTHER_LIBTOOL_OPT to that set.)

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.

None yet

4 participants