GeezerVison is showing github.com — gh-128354: Use `LIBS` consistently over `LDFLAGS` in library build checks by zanieb · Pull Request #128359 · python/cpython
Conversation
zanieb
commented
Dec 30, 2024
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
🤖 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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
That sounds good to me, though I think it's an independent change from this one.
Yes, I agree.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I think it would be safe to backport this, at least to 3.13, and possibly also 3.12. WDYT, @corona10 & @zanieb?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Yeah I think it is fine to backport the patch!!
|
erlend-aasland reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
|
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 :)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
zanieb
added a commit
to zanieb/cpython
that referenced
this pull request
Jan 4, 2025
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
zanieb
added a commit
to zanieb/cpython
that referenced
this pull request
Jan 4, 2025
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
corona10
Awaiting requested review from corona10
corona10 is a code owner
Uh oh!
There was an error while loading. Please reload this page.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|