Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-128354: Use LIBS consistently over LDFLAGS in library build checks #128359

Merged
merged 1 commit into from
Jan 3, 2025

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 bug and 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 <[email protected]>
@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 <[email protected]>
@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 bug and security fixes label Jan 4, 2025
WolframAlph pushed a commit to WolframAlph/cpython that referenced this pull request Jan 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants