Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-128354: UseLIBS consistently overLDFLAGS in library build checks#128359

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
erlend-aasland merged 1 commit intopython:mainfromzanieb:zb/build-libs
Jan 3, 2025

Conversation

@zanieb
Copy link
Contributor

@zaniebzanieb commentedDec 30, 2024•
edited
Loading

As noted in#128354, I audited all uses ofLIBS andLDFLAGS and adjusted checks using$<LIB>_LIBS to setLIBS instead ofLDFLAGS and ensured we consistently ordered$LIBS before$<LIB>_LIBS. There are some other cases where a constant is added toLIBS 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 ofCFLAGS andCPPFLAGS 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-buildbotsTest PR w/ buildbots; report in status section labelJan 3, 2025
@bedevere-bot
Copy link

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

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

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

In#95288, the ordering ofCFLAGS andCPPFLAGS 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 useCPPFLAGS 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 commentedJan 3, 2025•
edited
Loading

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

I remember having issues withsqlite3 dependency detection on a FreeBSD buildbot, because we initially usedCFLAGS instead ofCPPFLAGS.

@zanieb
Copy link
ContributorAuthor

In#95288, the ordering ofCFLAGS andCPPFLAGS 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 useCPPFLAGS 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 vsCPPFLAGS = $FOO $CPPFLAGS. I can look into that too though.

erlend-aasland reacted with thumbs up emoji

@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 commitb75ed95 intopython: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-aasland reacted with thumbs up emoji

@erlend-aaslanderlend-aasland added needs backport to 3.12only security fixes needs backport to 3.13bugs and security fixes labelsJan 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 requestJan 4, 2025
… build checks (pythonGH-128359)(cherry picked from commitb75ed95)Co-authored-by: Zanie Blue <contact@zanie.dev>
@bedevere-app
Copy link

GH-128465 is a backport of this pull request to the3.13 branch.

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

GH-128466 is a backport of this pull request to the3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelJan 4, 2025
WolframAlph pushed a commit to WolframAlph/cpython that referenced this pull requestJan 4, 2025
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull requestJan 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

@corona10corona10Awaiting requested review from corona10corona10 is a code owner

Assignees

@erlend-aaslanderlend-aasland

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zanieb@bedevere-bot@erlend-aasland@corona10
[8]ページ先頭
©2009-2025 Movatter.jp