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-114505: Add missing header file dependencies by smontanaro · Pull Request #114513 · python/cpython

Conversation

@smontanaro

@smontanaro smontanaro commented Jan 24, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

@colesbury and @erlend-aasland pointed out problems with some header file dependencies in Makefile.pre.in. This PR corrects the obvious problems, the definition of PYTHON_HEADERS and the dependencies for Programs/_testembed.o.

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

  • Issue: test_embed failures after switching configure options due to missing Makefile dependencies #114505

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@smontanaro
add some missing header file dependencies (probably more to be done, …
3ef6bb8 
…but these were called out for me)
@bedevere-app bedevere-app Bot added the awaiting review label Jan 24, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jan 24, 2024
test_embed failures after switching configure options due to missing Makefile dependencies #114505
Closed
@erlend-aasland
erlend-aasland self-requested a review January 24, 2024 07:58
@erlend-aasland erlend-aasland self-assigned this Jan 24, 2024
erlend-aasland
erlend-aasland reviewed Jan 24, 2024
View reviewed changes
Comment thread Makefile.pre.in Outdated
$(srcdir)/Include/cpython/pthread_stubs.h \
$(srcdir)/Include/cpython/pyatomic.h \
$(srcdir)/Include/cpython/pyatomic_gcc.h \
$(srcdir)/Include/cpython/pyatomic_msc.h \

@erlend-aasland erlend-aasland Jan 24, 2024

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.

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

Sorry, something went wrong.

Uh oh!

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

@smontanaro smontanaro Jan 24, 2024

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

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

@smontanaro smontanaro Jan 24, 2024

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland Jan 25, 2024

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.

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

I guess 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 mention configure.ac.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Makefile.pre.in Outdated
$(srcdir)/Include/codecs.h \
$(srcdir)/Include/compile.h \
$(srcdir)/Include/complexobject.h \
$(srcdir)/Include/datetime.h \

@erlend-aasland erlend-aasland Jan 24, 2024

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland added the skip news label Jan 24, 2024
@smontanaro
changes based on feedback by Erlend Aasland
1a0763e
@smontanaro

smontanaro commented Jan 24, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury
colesbury reviewed Jan 24, 2024
View reviewed changes
Comment thread Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.
@smontanaro
move PYTHON_HEADERS defn up and make _testembed.o depend on it
47707fc
@smontanaro

smontanaro commented Jan 25, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

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

colesbury commented Jan 25, 2024

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

Sorry, something went wrong.

Uh oh!

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

smontanaro commented Jan 25, 2024

Copy link
Copy Markdown
Contributor Author

I don't understand how to fix that merge conflict. I moved the def'n of PYTHON_HEADERS up as you suggested, but it appears you added pycore_gc.h to PYTHON_HEADERS on main.

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

Sorry, something went wrong.

Uh oh!

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

colesbury commented Jan 25, 2024

Copy link
Copy Markdown
Contributor

Sorry about that. It looks like my PR added pycore_gc.h and pycore_object_stack.h. Here's the diff 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 into issue114505)

And then resolve the merge conflict (make sure that pycore_gc.h and pycore_object_stack.h are listed in PYTHON_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
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 25, 2024

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
erlend-aasland requested a review from zware January 25, 2024 23:10
@smontanaro
complete merge
bc74ade
@smontanaro

smontanaro commented Jan 26, 2024

Copy link
Copy Markdown
Contributor Author

Sorry about that. It looks like my PR added pycore_gc.h and pycore_object_stack.h. Here's the diff of Makefile.pre.in since the commit you are based on.

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

Sorry, something went wrong.

Uh oh!

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

smontanaro commented Feb 4, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland approved these changes Feb 7, 2024
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.

Thanks! I'll resolve the conflicts before landing.

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 Feb 7, 2024
@erlend-aasland
Pull in main
a551a25
@erlend-aasland
erlend-aasland enabled auto-merge (squash) February 7, 2024 08:33
@erlend-aasland erlend-aasland linked an issue Feb 7, 2024 that may be closed by this pull request
test_embed failures after switching configure options due to missing Makefile dependencies #114505
Closed
@erlend-aasland
erlend-aasland merged commit 2afc718 into python:main Feb 7, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 7, 2024
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
@smontanaro @fsc-eriker
pythongh-114505: Add missing header file dependencies (python#114513)
2a0f89d 
Also move PYTHON_HEADERS up and make _testembed.o depend on it.
Bonniemarie216
Bonniemarie216 approved these changes Sep 15, 2024
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury left review comments
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@zware zware Awaiting requested review from zware
+1 more reviewer
@Bonniemarie216 Bonniemarie216 Bonniemarie216 approved these changes
Reviewers whose approvals may not affect merge requirements

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.

test_embed failures after switching configure options due to missing Makefile dependencies

Uh oh!

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

4 participants

@smontanaro @colesbury @erlend-aasland @Bonniemarie216
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.