GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

@zanieb zanieb commented Dec 30, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

All reactions
@zanieb
Use LIBS consistently over LDFLAGS in library build checks
a07b043
@bedevere-app bedevere-app Bot mentioned this pull request Dec 30, 2024
LDFLAGS and LIBS not used consistently in configure #128354
Closed
@zanieb
zanieb marked this pull request as ready for review January 3, 2025 03:04
@zanieb
zanieb requested review from corona10 and erlend-aasland as code owners January 3, 2025 03:04
@bedevere-app bedevere-app Bot added the awaiting review label Jan 3, 2025
@erlend-aasland erlend-aasland added the skip news label Jan 3, 2025
erlend-aasland
erlend-aasland approved these changes Jan 3, 2025
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

Great, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jan 3, 2025
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 3, 2025
@bedevere-bot

bedevere-bot commented Jan 3, 2025

Copy link
Copy Markdown

🤖 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

erlend-aasland commented Jan 3, 2025

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jan 3, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zanieb

zanieb commented Jan 3, 2025

Copy link
Copy Markdown
Contributor Author

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
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jan 3, 2025

Copy link
Copy Markdown
Contributor

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

Yes, I agree.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
erlend-aasland merged commit b75ed95 into python:main Jan 3, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 3, 2025
@erlend-aasland

erlend-aasland commented Jan 3, 2025

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10

corona10 commented Jan 4, 2025

Copy link
Copy Markdown
Member

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

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

This comment was marked as outdated.

Sign in to view
@miss-islington-app

This comment was marked as outdated.

Sign in to view
@miss-islington-app

This comment has been minimized.

Sign in to view
@miss-islington-app miss-islington-app Bot assigned erlend-aasland Jan 4, 2025
@miss-islington-app

This comment has been minimized.

Sign in to view
@erlend-aasland

erlend-aasland commented Jan 4, 2025

Copy link
Copy Markdown
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 :)

All reactions

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
@zanieb
[3.13] pythongh-128354: Consistently use LIBS over LDFLAGS in library…
f6b30a5
… build checks (pythonGH-128359)

(cherry picked from commit b75ed95)

Co-authored-by: Zanie Blue <[email protected]>
@zanieb zanieb mentioned this pull request Jan 4, 2025
[3.13] gh-128354: Consistently use LIBS over LDFLAGS in library build… #128465
Merged
@bedevere-app

bedevere-app Bot commented Jan 4, 2025

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Jan 4, 2025
zanieb added a commit to zanieb/cpython that referenced this pull request Jan 4, 2025
@zanieb
[3.12] pythongh-128354: Consistently use LIBS over LDFLAGS in library…
789f6cf
… build checks (pythonGH-128359)

(cherry picked from commit b75ed95)

Co-authored-by: Zanie Blue <[email protected]>
@zanieb zanieb mentioned this pull request Jan 4, 2025
[3.12] gh-128354: Consistently use LIBS over LDFLAGS in library build… #128466
Merged
@bedevere-app

bedevere-app Bot commented Jan 4, 2025

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
@zanieb @WolframAlph
pythongh-128354: Consistently use LIBS over LDFLAGS in library build …
29bd2e5
…checks (python#128359)
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
@zanieb @srinivasreddy
pythongh-128354: Consistently use LIBS over LDFLAGS in library build …
7d4e098
…checks (python#128359)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland approved these changes
@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@erlend-aasland erlend-aasland

Labels

skip news

Projects

None yet

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.

4 participants

@zanieb @bedevere-bot @erlend-aasland @corona10
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.