gh-104523: Use dynamic rule for compiling libmpdec and libexpat by indygreg · Pull Request #104574 · python/cpython

indygreg · 2023-05-17T04:14:07Z

Before we had a separate rule for each source/object file. We can use $(foreach $(eval $(call ...))) to dynamically derive rules to avoid the repetition.

I think this is sufficiently portable. If not, there are alternatives to $(patsubst) that can be leveraged.

Issue: Overhaul build system rules related to profiling #104523

Before we had a separate rule for each source/object file. We can use `$(foreach $(eval $(call ...)))` to dynamically derive rules to avoid the repetition. skip news (behavior preserving build system change)

erlend-aasland · 2023-05-18T21:18:10Z

cc. also @ned-deily who has opinions about the build process.

indygreg · 2023-05-18T21:39:04Z

FWIW I have a follow-up change to the module freezing code that is a net -100 lines on the diff which uses a similar approach to reduce make boilerplate and redundancy. That one more clearly demonstrates value in the general pattern than this one. But the justifications are basically the same.

zware · 2023-05-18T22:19:13Z

Is this really more portable than the pattern rule approach? It's certainly more cryptic :)

In these particular cases, I'd personally like to work towards getting libmpdec and expat out of our repository entirely (i.e., make --with-system-expat --with-system-libmpdec the default and later only way to build), though that's a bigger project with a longer timeline. I wonder if it would make sense to pull in a bit extra from these packages for now in order to delegate to their own build system to build them instead of shoehorning them into ours, but a cursory look says that's probably more effort than it's worth.

This looks reasonable enough to try it on the buildbots, at least.

bedevere-bot · 2023-05-18T22:19:27Z

🤖 New build scheduled with the buildbot fleet by @zware for commit 44eda25 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

zware · 2023-05-19T16:31:59Z

The buildbot failures don't look related to this change.

ned-deily · 2023-05-19T18:40:08Z

The buildbot failures don't look related to this change.

@zware, it looks like the FreeBSD buildbot build didn't or hasn't run yet? I think this is the relevant buildrequest.

zware · 2023-05-19T18:45:57Z

That worker has apparently been down for about 4 months now, though I can't tell at a glance if the fault lies on the client or server side (@koobs?)

erlend-aasland · 2023-05-20T15:47:17Z

IMO, we should wait for 3.13 to open before landing this.

indygreg · 2023-05-20T23:19:45Z

I didn't realize we were only days away from the end of the 3.12 new features window. I guess I'm so used to Rust and its 6 week release cadence.

Anyway, yes, I'm fine waiting until the 3.13 window to land more invasive build system changes. (Not like I have much choice in the matter.)

erlend-aasland · 2023-06-22T10:21:24Z

@indygreg: How does this fare on BSD Make?

vstinner · 2023-09-04T10:11:57Z

On my FreeBSD, I can no longer build Python :-(

$ make
make: "/usr/home/vstinner/python/main/Makefile" line 1047: Invalid line type
make: "/usr/home/vstinner/python/main/Makefile" line 1050: Invalid line type
make: "/usr/home/vstinner/python/main/Makefile" line 1052: Invalid line type
make: "/usr/home/vstinner/python/main/Makefile" line 1062: Invalid line type
make: "/usr/home/vstinner/python/main/Makefile" line 1065: Invalid line type
make: "/usr/home/vstinner/python/main/Makefile" line 1067: Invalid line type
make: Fatal errors encountered -- cannot continue
make: stopped in /usr/home/vstinner/python/main

I have;

$ make -V MAKE_VERSION
20220208

erlend-aasland · 2023-09-04T10:21:34Z

On my FreeBSD, I can no longer build Python :-(

Could you build it on FreeBSD before this PR?

vstinner · 2023-09-04T22:25:06Z

Could you build it on FreeBSD before this PR?

Sorry. I mean: currently, on the main branch of Python (without this PR), I can no longer build Python :-(

erlend-aasland · 2023-09-05T07:26:52Z

Could you build it on FreeBSD before this PR?
Sorry. I mean: currently, on the main branch of Python (without this PR), I can no longer build Python :-(

Aha, so this PR does not change the status quo! I suspected that. I guess we should clarify if was want to restore BSD Make compatibility, or carry on with GNU Make only.

vstinner · 2023-09-05T16:52:25Z

Please fix building Python on FreeBSD before making the situation worse.

vstinner · 2023-09-05T16:53:13Z

Sadly, the maintainer of our two FreBSD decided to remove them. Right now, there is no FreeBSD CI. We should consider CirrusCI.

vstinner · 2023-09-05T18:33:03Z

I tested FreeBSD again, and I'm sorry, I didn't test make correctly previously.

Building Python in the main branch on FreeBSD is fine!
Building Python with this PR on FreeBSD no longer works

This PR introduces these issues in Makefile:

$ make
make: "/usr/home/vstinner/python/main/Makefile" line 1047: Invalid line type
make: "/usr/home/vstinner/python/main/Makefile" line 1050: Invalid line type
make: "/usr/home/vstinner/python/main/Makefile" line 1052: Invalid line type
make: "/usr/home/vstinner/python/main/Makefile" line 1062: Invalid line type
make: "/usr/home/vstinner/python/main/Makefile" line 1065: Invalid line type
make: "/usr/home/vstinner/python/main/Makefile" line 1067: Invalid line type
make: Fatal errors encountered -- cannot continue
make: stopped in /usr/home/vstinner/python/main

Surprise, surprise, it's the "non portable" part of this PR which introduces the issue. Like:

# "%.o: %c" is not portable
define libmpdec_compile
$(1): $$(srcdir)/$$(patsubst %.o,%.c,$(1)) $$(LIBMPDEC_HEADERS) $$(PYTHON_HEADERS)
$$(CC) -c $$(LIBMPDEC_CFLAGS) -o $$@ $$<
endef

vstinner

While it's appealing to use cool GNU make features to make the Makefile shorter, I don't think that it's worth it if in exchange we break FreeBSD support.

FreeBSD is a Tier-3 platform: https://peps.python.org/pep-0011/

Is there another command which is compatible with GNU make and bsdmake?

bedevere-bot · 2023-09-05T18:34:18Z

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.

And if you don't make the requested changes, you will be put in the comfy chair!

vstinner · 2023-10-04T10:56:43Z

@erlend-aasland: Is this change worth it if it breaks FreeBSD support? Should we just close it?

bedevere-bot · 2023-10-04T11:00:41Z

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 444a7c1 🤖

The command will test the builders whose names match following regular expression: .*freebsd.*

The builders matched are:

AMD64 FreeBSD PR

erlend-aasland · 2023-10-04T11:03:30Z

@erlend-aasland: Is this change worth it if it breaks FreeBSD support? Should we just close it?

If it breaks FreeBSD support it is not worth it. There might be a more portable approach, though. cc. @indygreg

vstinner · 2023-10-04T11:11:56Z

If it breaks FreeBSD support it is not worth it.

Do you expect that FreeBSD buildbot will behave differently than my previous manual test?
#104574 (comment)

The PR didn't change in the meanwhile.

There might be a more portable approach, though. cc. @indygreg

I'm fine with any syntax as soon as it works on all platforms supported by Python ;-)

erlend-aasland · 2023-10-04T11:17:13Z

Do you expect that FreeBSD buildbot will behave differently than my previous manual test? #104574 (comment)
The PR didn't change in the meanwhile.

Oh, I forgot you already tested this!

vstinner · 2023-10-04T12:00:31Z

buildbot/AMD64 FreeBSD PR — Build done.

Fail to build:

 using PTY: False
make: "/buildbot/buildarea/pull_request.ware-freebsd/build/Makefile" line 1049: Invalid line type
make: "/buildbot/buildarea/pull_request.ware-freebsd/build/Makefile" line 1052: Invalid line type
make: "/buildbot/buildarea/pull_request.ware-freebsd/build/Makefile" line 1054: Invalid line type
make: "/buildbot/buildarea/pull_request.ware-freebsd/build/Makefile" line 1064: Invalid line type
make: "/buildbot/buildarea/pull_request.ware-freebsd/build/Makefile" line 1067: Invalid line type
make: "/buildbot/buildarea/pull_request.ware-freebsd/build/Makefile" line 1069: Invalid line type
make: stopped in /buildbot/buildarea/pull_request.ware-freebsd/build
make: Fatal errors encountered -- cannot continue

indygreg · 2023-10-04T12:07:01Z

There’s a gmake port on FreeBSD, no? What’s wrong with requiring people to use gmake? Are there actually platforms where GNU Make isn’t trivially installable?

vstinner · 2023-10-05T08:50:57Z

What’s wrong with requiring people to use gmake?

It makes building Python on FreBSD more annoying and complicated.

vstinner · 2023-10-05T08:51:58Z

If someone wants to use more advanced Makefile rule, I suggest to rewrite the whole Python build system with CMake, Meson or anything else.

github-actions · 2026-04-13T00:23:06Z

This PR is stale because it has been open for 30 days with no activity.

koobs · 2026-04-14T05:59:56Z

unstale

vstinner · 2026-04-14T10:42:58Z

This change breaks support for BSD make on FreeBSD. I don't think that it's worth it, so I prefer to close it.

@zware:

In these particular cases, I'd personally like to work towards getting libmpdec and expat out of our repository entirely

For libmpdec, there is a concrete plan for that: #115119. This PR is mostly about libmpdec, it only changes 6 lines about expat.

Building Python with this PR on FreeBSD no longer works
(...)
While it's appealing to use cool GNU make features to make the Makefile shorter, I don't think that it's worth it if in exchange we break FreeBSD support.

If someone disagrees, please open a discussion on discuss.python.org suggesting to drop support for BSD make and require GNU make on FreeBSD.

There are also generic discussions about changing the whole Python build system, such as What do you want to see in tomorrow’s CPython build system?

@indygreg:

I think this is sufficiently portable. If not, there are alternatives to $(patsubst) that can be leveraged.

If someone wants to propose a PR using $(patsubst), please go ahead. I can test it on FreeBSD.

Another approach would be to generate the code for libmpdec, as we already do to build stdlib extension modules. (I'm not sure which code is involved, maybe Modules/makesetup.)

bedevere-bot added the awaiting review label May 17, 2023

bedevere-bot mentioned this pull request May 17, 2023
Overhaul build system rules related to profiling #104523
Closed

pythongh-104523: use dynamic rule for compiling libmpdec and libexpat
44eda25
Before we had a separate rule for each source/object file. We can use `$(foreach $(eval $(call ...)))` to dynamically derive rules to avoid the repetition. skip news (behavior preserving build system change)

indygreg force-pushed the dynamic-build-rules branch from 0c12c20 to 44eda25Compare May 18, 2023 00:12

erlend-aasland requested review from erlend-aasland and zware May 18, 2023 21:15

erlend-aasland changed the title ~~gh-104523: use dynamic rule for compiling libmpdec and libexpat~~gh-104523: Use dynamic rule for compiling libmpdec and libexpatMay 18, 2023

erlend-aasland added the skip news label May 18, 2023

zware added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 18, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 18, 2023

Merge branch 'main' into dynamic-build-rules
38b0c37

Merge branch 'main' into dynamic-build-rules
d3cbd2b

vstinner requested changes Sep 5, 2023
View reviewed changes

bedevere-bot removed the awaiting review label Sep 5, 2023

bedevere-bot added the awaiting changes label Sep 5, 2023

vstinner mentioned this pull request Sep 5, 2023
Add FreeBSD build and test using Cirrus-CI #91960
Closed

Merge branch 'main' into dynamic-build-rules
444a7c1

This comment was marked as outdated.
Sign in to view

This comment was marked as duplicate.
Sign in to view

encukou added the build The build process and cross-build label Mar 8, 2024

github-actionsBot added the stale Stale PR or inactive for long period of time. label Apr 13, 2026

vstinner closed this Apr 14, 2026

Uh oh!

Uh oh!

Conversation

indygreg commented May 17, 2023• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented May 18, 2023

Uh oh!

indygreg commented May 18, 2023

Uh oh!

zware commented May 18, 2023

Uh oh!

bedevere-bot commented May 18, 2023

Uh oh!

zware commented May 19, 2023

Uh oh!

ned-deily commented May 19, 2023

Uh oh!

zware commented May 19, 2023

Uh oh!

erlend-aasland commented May 20, 2023

Uh oh!

indygreg commented May 20, 2023

Uh oh!

erlend-aasland commented Jun 22, 2023

Uh oh!

vstinner commented Sep 4, 2023

Uh oh!

erlend-aasland commented Sep 4, 2023

Uh oh!

vstinner commented Sep 4, 2023

Uh oh!

erlend-aasland commented Sep 5, 2023

Uh oh!

vstinner commented Sep 5, 2023

Uh oh!

vstinner commented Sep 5, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Sep 5, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 5, 2023

Uh oh!

vstinner commented Oct 4, 2023

Uh oh!

This comment was marked as outdated.

bedevere-bot commented Oct 4, 2023

Uh oh!

This comment was marked as outdated.

This comment was marked as duplicate.

erlend-aasland commented Oct 4, 2023

Uh oh!

vstinner commented Oct 4, 2023

Uh oh!

erlend-aasland commented Oct 4, 2023

Uh oh!

vstinner commented Oct 4, 2023

Uh oh!

indygreg commented Oct 4, 2023 via email

Uh oh!

vstinner commented Oct 5, 2023

Uh oh!

vstinner commented Oct 5, 2023

Uh oh!

github-actionsBot commented Apr 13, 2026

Uh oh!

koobs commented Apr 14, 2026

Uh oh!

vstinner commented Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

indygreg commented May 17, 2023•
edited by bedevere-bot
Loading

vstinner commented Sep 5, 2023•
edited
Loading