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

Merged
merged 1 commit into from
Jan 22, 2023
Merged

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 indygreg force-pushed the remove-libtool-cruft branch from ab9ca85 to a1016d2 Compare January 15, 2023 16:12
@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.

@indygreg indygreg force-pushed the remove-libtool-cruft branch from a1016d2 to dae8974 Compare January 15, 2023 16:15
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.)

@ned-deily
Copy link
Member

I agree with the desire to avoid "scope bloat" but I think the key here is what is the reason for making this change at all. In general, we don't make code cleanup changes without a reason. But this PR identifies a good reason, that is, the commit you cited, 1919983, was incomplete; since it removed the last use of libtool for macOS builds, it should have removed the remaining references to it. The PR as it stands removes one of those; a quick grep of the source identifies the other, the OTHER_LIBTOOL_OPT variable (also seen in the cited commit). So the reason for making this change is to remove libtool cruft, not just LIBTOOL_CRUFT, and, if we make such a change, we need to finish the job not just leave it partially done. It's pretty clear that this is a low-risk change but we would further mitigate risk of problems by only making such a change in the main branch for the next feature release; i.e. we would not backport it to existing releases as it fixes no bugs.

Both variables have 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.
@indygreg indygreg force-pushed the remove-libtool-cruft branch from dae8974 to 44d2741 Compare January 21, 2023 03:46
@indygreg
Copy link
Contributor Author

I refreshed the commit to also delete code related to OTHER_LIBTOOL_OPT.

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 making the changes!

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.

4 participants