Skip to content

Base header #3486

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

oddbookworm
Copy link
Member

Another precursor PR to #3477
The reason I want to remove so much static linkage is because you can't test functions that aren't visible to you (not even worth putting those declarations in the header, since anyone that includes the header would need to provide their own implementation to call them).

@oddbookworm oddbookworm requested a review from a team as a code owner June 8, 2025 00:36
oddbookworm added a commit that referenced this pull request Jun 9, 2025
@gresm
Copy link
Contributor

gresm commented Jun 9, 2025

You could also do in base_test.c:

#include "base.c"

[...]

But that'll make building tests messy...

@oddbookworm
Copy link
Member Author

You could also do in base_test.c:

#include "base.c"

Absolutely not

That begs for linker problems that would be a pain to solve, and ultimately, having a header with all the declarations is just cleaner IMO. Including a source file is just plain nasty

Copy link
Member

@pmp-p pmp-p left a comment

Choose a reason for hiding this comment

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

there are problems that could be solved i thing by simply merging static and base ( ie when wasm loading pygame_base static could be set to pygame.base by pygame/__init__.py

// in case of wasm+dynamic loading it could be a trampoline in the globals
// generated at runtime.
// when building static make global accessible symbol directly.
extern PyObject *pgExc_SDLError;
Copy link
Member

@pmp-p pmp-p Jun 28, 2025

Choose a reason for hiding this comment

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

pgExc_SDLError is not defined anymore but it must be a global in the first C module loaded (afaik pygame.base but pygame_static in wasm).


/* Custom exceptions */
static PyObject *pgExc_BufferError = NULL;
PyObject *pgExc_BufferError = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

same as pgExc_SDLError, this must be in toplevel C module.

src_c/base.c Outdated
static PyObject *pgExc_BufferError = NULL;
PyObject *pgExc_BufferError = NULL;

PyObject *pgExc_SDLError;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be global only in the wasm codepath? The same symbol gets reused in the base init function. Alternatively, removing the declaration there could also work.

Copy link
Member

@pmp-p pmp-p Jul 1, 2025

Choose a reason for hiding this comment

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

pgExc_SDLError is used everywhere, imho it should not be defined in base init but globally- wasm or not - since it goes out of scope while being exported in c-api pointers

c_api[0] = pgExc_SDLError;

@pmp-p
Copy link
Member

pmp-p commented Jul 1, 2025

last commit won't do it : wasm at runtime says that pgExc_SDLError needs to be globally exported for dynamic loading.

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