Skip to content

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Dec 30, 2024

As noted in #128354, I audited all uses of LIBS and LDFLAGS and adjusted checks using $<LIB>_LIBS to set LIBS instead of LDFLAGS and ensured we consistently ordered $LIBS before $<LIB>_LIBS. There are some other cases where a constant is added to LIBS that I did not change here since it's a different pattern — we can consider those separately.

I don't believe this needs a news entry, but defer to whatever the reviewer prefers.

I tested this locally on macOS and in a Linux container. It seems nice to get more test coverage too, perhaps via the build-bots?

In #95288, the ordering of CFLAGS and CPPFLAGS was fixed in a similar manner. I also noticed that some of these uses were introduced in #94802.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 3, 2025
@bedevere-bot
Copy link

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 3, 2025
@erlend-aasland
Copy link
Contributor

In #95288, the ordering of CFLAGS and CPPFLAGS was fixed in a similar manner. I also noticed that some of these uses were introduced in #94802.

IMO, the safest thing to do is to consistently use CPPFLAGS iso. CFLAGS; CPPFLAGS will work with both pre-processor and compiler checks; CFLAGS may not work with pre-processor checks.

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jan 3, 2025

IMO, the safest thing to do is to consistently use CPPFLAGS iso. CFLAGS; CPPFLAGS will work with both pre-processor and compiler checks; CFLAGS may not work with pre-processor checks.

I remember having issues with sqlite3 dependency detection on a FreeBSD buildbot, because we initially used CFLAGS instead of CPPFLAGS.

@zanieb
Copy link
Contributor Author

zanieb commented Jan 3, 2025

In #95288, the ordering of CFLAGS and CPPFLAGS was fixed in a similar manner. I also noticed that some of these uses were introduced in #94802.

IMO, the safest thing to do is to consistently use CPPFLAGS iso. CFLAGS; CPPFLAGS will work with both pre-processor and compiler checks; CFLAGS may not work with pre-processor checks.

That sounds good to me, though I think it's an independent change from this one. I'm just referring to the ordering when extending flags, e.g., CPPFLAGS = $CPPFLAGS $FOO vs CPPFLAGS = $FOO $CPPFLAGS. I can look into that too though.

@erlend-aasland
Copy link
Contributor

That sounds good to me, though I think it's an independent change from this one.

Yes, I agree.

@erlend-aasland erlend-aasland merged commit b75ed95 into python:main Jan 3, 2025
124 checks passed
@erlend-aasland
Copy link
Contributor

I think it would be safe to backport this, at least to 3.13, and possibly also 3.12. WDYT, @corona10 & @zanieb?

@corona10
Copy link
Member

corona10 commented Jan 4, 2025

Yeah I think it is fine to backport the patch!!

@erlend-aasland erlend-aasland added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jan 4, 2025
@miss-islington-app

This comment was marked as outdated.

@miss-islington-app

This comment was marked as outdated.

@miss-islington-app

This comment has been minimized.

@miss-islington-app

This comment has been minimized.

@erlend-aasland
Copy link
Contributor

Well, it does not apply cleanly; I don't think it is worth the effort to manually backport this, but if someone wants to take it on, feel free to do so :)

zanieb added a commit to zanieb/cpython that referenced this pull request Jan 4, 2025
… build checks (pythonGH-128359) (cherry picked from commit b75ed95) Co-authored-by: Zanie Blue <contact@zanie.dev>
@bedevere-app
Copy link

bedevere-app bot commented Jan 4, 2025

GH-128465 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jan 4, 2025
zanieb added a commit to zanieb/cpython that referenced this pull request Jan 4, 2025
… build checks (pythonGH-128359) (cherry picked from commit b75ed95) Co-authored-by: Zanie Blue <contact@zanie.dev>
@bedevere-app
Copy link

bedevere-app bot commented Jan 4, 2025

GH-128466 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Jan 4, 2025
WolframAlph pushed a commit to WolframAlph/cpython that referenced this pull request Jan 4, 2025
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

4 participants