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-114505: Add missing header file dependencies#114513

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 5 commits intopython:mainfromsmontanaro:issue114505
Feb 7, 2024

Conversation

@smontanaro
Copy link
Contributor

@smontanarosmontanaro commentedJan 24, 2024•
edited by bedevere-appbot
Loading

@colesbury and@erlend-aasland pointed out problems with some header file dependencies inMakefile.pre.in. This PR corrects the obvious problems, the definition ofPYTHON_HEADERS and the dependencies forPrograms/_testembed.o.

I believe the various*_HEADERS definitions are now correct (I found nothing else amiss besidesPYTHON_HEADERS). I've not yet looked at any other.o -> .h dependencies, but will do that.

$(srcdir)/Include/cpython/pthread_stubs.h \
$(srcdir)/Include/cpython/pyatomic.h \
$(srcdir)/Include/cpython/pyatomic_gcc.h \
$(srcdir)/Include/cpython/pyatomic_msc.h \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Isn't this Windows only? If so, we can probably discard it here in the *nix build.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I agree I was probably overzealous. I'm happy to have some extra dependencies, false positives being better than false negatives in this case. I'll correct my overzealousness.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Rhetorical question: Why don't we use GCC, CLANG, or other compilers' ability to generate include dependencies? I'm thinking stuff likegcc -M orclang -M.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Rhetorical question: Why don't we use GCC, CLANG, or other compilers' ability to generate include dependencies? I'm thinking stuff likegcc -M orclang -M.

Iguess just because the current build system has grown into something that is very hard to maintain, so people in general just try to make as few changes as possible, if any at all. Not to mentionconfigure.ac.

$(srcdir)/Include/codecs.h \
$(srcdir)/Include/compile.h \
$(srcdir)/Include/complexobject.h \
$(srcdir)/Include/datetime.h \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is for the datetime C API capsule. Perhaps this dependency should be more targeted.

@smontanaro
Copy link
ContributorAuthor

Made the changes Erlend requested on MacOS. Pushed and will now test on Linux.

@smontanaro
Copy link
ContributorAuthor

Any further inputs on this, or can it move toward acceptance and merging?

@colesbury
Copy link
Contributor

I don't have any more comments. The change looks good to me, although there is a merge conflict that needs to be resolved.

@smontanaro
Copy link
ContributorAuthor

I don't understand how to fix that merge conflict. I moved the def'n ofPYTHON_HEADERS up as you suggested, but it appears you addedpycore_gc.h toPYTHON_HEADERS onmain.

I've been chastised for merging frommain to a PR in the past. Is this a case where it's necessary?

@colesbury
Copy link
Contributor

Sorry about that. It looks like my PR addedpycore_gc.h andpycore_object_stack.h. Here's thediff of Makefile.pre.in since the commit you are based on.

In my experience, merging "main" into the PR branch is okay. Sometimes people accidentally merge in the other direction and that can lead to a giant mess.

@erlend-aasland might have better ideas, but my suggestion would be (if using git from the command line):

  1. git checkout main
  2. git pull (update your local main branch)
  3. git checkout issue114505
  4. git merge main (merge main intoissue114505)

And then resolve the merge conflict (make sure thatpycore_gc.h andpycore_object_stack.h are listed inPYTHON_HEADERS).

Let me know if you'd like help with it. I think I have permissions on GitHub to resolve the merge conflict and push to the branch, if you'd like me to do it.

erlend-aasland reacted with thumbs up emoji

@erlend-aasland
Copy link
Contributor

Thanks! I'll take a closer look at this, hopefully before the coming weekend is over.

@smontanaro
Copy link
ContributorAuthor

Sorry about that. It looks like my PR addedpycore_gc.h andpycore_object_stack.h. Here's thediff of Makefile.pre.in since the commit you are based on.

Thanks. I followed your instructions and resolved the conflict inMakefile.pre.in. Hopefully I did it right...

@smontanaro
Copy link
ContributorAuthor

What's the status on this? I thought I'd resolved the conflicts a week ago. Should I start over?

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.

Thanks! I'll resolve the conflicts before landing.

@erlend-aaslanderlend-aaslandenabled auto-merge (squash)February 7, 2024 08:33
@erlend-aaslanderlend-aasland merged commit2afc718 intopython:mainFeb 7, 2024
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull requestFeb 14, 2024
Also move PYTHON_HEADERS up and make _testembed.o depend on it.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@colesburycolesburycolesbury left review comments

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

@zwarezwareAwaiting requested review from zware

+1 more reviewer

@Bonniemarie216Bonniemarie216Bonniemarie216 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@erlend-aaslanderlend-aasland

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test_embed failures after switching configure options due to missing Makefile dependencies

4 participants

@smontanaro@colesbury@erlend-aasland@Bonniemarie216
[8]ページ先頭
©2009-2025 Movatter.jp