gh-104523: Use dynamic rule for compiling libmpdec and libexpat #104574

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.

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

Open

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 44eda25 Compare May 18, 2023 00:12

erlend-aasland requested review from zware and erlend-aasland 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 libexpat May 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-104523: Use dynamic rule for compiling libmpdec and libexpat #104574

gh-104523: Use dynamic rule for compiling libmpdec and libexpat #104574

indygreg commented May 17, 2023 •

edited by bedevere-bot

Loading

erlend-aasland commented May 18, 2023

indygreg commented May 18, 2023

zware commented May 18, 2023

bedevere-bot commented May 18, 2023

zware commented May 19, 2023

ned-deily commented May 19, 2023

zware commented May 19, 2023

erlend-aasland commented May 20, 2023

indygreg commented May 20, 2023

erlend-aasland commented Jun 22, 2023

vstinner commented Sep 4, 2023

erlend-aasland commented Sep 4, 2023

vstinner commented Sep 4, 2023

erlend-aasland commented Sep 5, 2023

vstinner commented Sep 5, 2023

vstinner commented Sep 5, 2023 •

edited

Loading

vstinner commented Sep 5, 2023

vstinner left a comment

bedevere-bot commented Sep 5, 2023

vstinner commented Oct 4, 2023

This comment was marked as outdated.

bedevere-bot commented Oct 4, 2023

This comment was marked as outdated.

This comment was marked as duplicate.

erlend-aasland commented Oct 4, 2023

vstinner commented Oct 4, 2023

erlend-aasland commented Oct 4, 2023

vstinner commented Oct 4, 2023

indygreg commented Oct 4, 2023 via email

vstinner commented Oct 5, 2023

vstinner commented Oct 5, 2023

gh-104523: Use dynamic rule for compiling libmpdec and libexpat #104574

Are you sure you want to change the base?

gh-104523: Use dynamic rule for compiling libmpdec and libexpat #104574

Conversation

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

erlend-aasland commented May 18, 2023

indygreg commented May 18, 2023

zware commented May 18, 2023

bedevere-bot commented May 18, 2023

zware commented May 19, 2023

ned-deily commented May 19, 2023

zware commented May 19, 2023

erlend-aasland commented May 20, 2023

indygreg commented May 20, 2023

erlend-aasland commented Jun 22, 2023

vstinner commented Sep 4, 2023

erlend-aasland commented Sep 4, 2023

vstinner commented Sep 4, 2023

erlend-aasland commented Sep 5, 2023

vstinner commented Sep 5, 2023

vstinner commented Sep 5, 2023 • edited Loading

vstinner commented Sep 5, 2023

vstinner left a comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 5, 2023

vstinner commented Oct 4, 2023

This comment was marked as outdated.

bedevere-bot commented Oct 4, 2023

This comment was marked as outdated.

This comment was marked as duplicate.

erlend-aasland commented Oct 4, 2023

vstinner commented Oct 4, 2023

erlend-aasland commented Oct 4, 2023

vstinner commented Oct 4, 2023

indygreg commented Oct 4, 2023 via email

vstinner commented Oct 5, 2023

vstinner commented Oct 5, 2023

indygreg commented May 17, 2023 •

edited by bedevere-bot

Loading

vstinner commented Sep 5, 2023 •

edited

Loading