Skip to content

Conversation

@zanieb
Copy link
Contributor

@zaniebzanieb 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-aaslanderlend-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-aaslanderlend-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-botbedevere-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
ContributorAuthor

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-aaslanderlend-aasland merged commit b75ed95 into python:mainJan 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

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

@erlend-aaslanderlend-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

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

@bedevere-appbedevere-appbot 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

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

@bedevere-appbedevere-appbot 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@zanieb@bedevere-bot@erlend-aasland@corona10