Skip to content

bpo-35081: Rename internal headers #10275

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

Merged
merged 2 commits into from
Nov 12, 2018
Merged

bpo-35081: Rename internal headers #10275

merged 2 commits into from
Nov 12, 2018

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 1, 2018

Rename Include/internal/ headers:

  • pycore_hash.h -> pycore_pyhash.h
  • pycore_lifecycle.h -> pycore_pylifecycle.h
  • pycore_mem.h -> pycore_pymem.h
  • pycore_state.h -> pycore_pystate.h

Add missing headers to Makefile.pre.in and PCbuild:

  • pycore_condvar.h.
  • pycore_hamt.h
  • pycore_pyhash.h

https://bugs.python.org/issue35081

@serhiy-storchaka
Copy link
Member

Why not use just the underscore prefix for distinguishing from public names, as common in Python?

@vstinner
Copy link
Member Author

vstinner commented Nov 1, 2018

Why not use just the underscore prefix for distinguishing from public names, as common in Python?

I plan to add one or two more subdirectories, so I will need more distinct prefixes, especially for "#ifndef Py_LIMITED_API" code. See:
https://bugs.python.org/issue35081#msg328619

@vstinner
Copy link
Member Author

vstinner commented Nov 1, 2018

I plan to add one or two more subdirectories, so I will need more distinct prefixes, especially for "#ifndef Py_LIMITED_API" code.

I created https://bugs.python.org/issue35134 and PR #10285 which should show better what I would like to do.

@vstinner
Copy link
Member Author

vstinner commented Nov 6, 2018

@nascheme, @serhiy-storchaka, @zooba, @ncoghlan: Would you mind to review this PR? I would prefer to not rename/move files multiple times :-)

@vstinner
Copy link
Member Author

vstinner commented Nov 8, 2018

Ah. There are now conflicts. I will fix them when this PR will be approved.

Rename Include/internal/ headers:

* pycore_hash.h -> pycore_pyhash.h
* pycore_lifecycle.h -> pycore_pylifecycle.h
* pycore_mem.h -> pycore_pymem.h
* pycore_state.h -> pycore_pystate.h

Add missing headers to Makefile.pre.in and PCbuild:

* pycore_condvar.h.
* pycore_hamt.h
* pycore_pyhash.h
@vstinner
Copy link
Member Author

vstinner commented Nov 9, 2018

I plan to merge this change next week, if I don't get any feedback in the meanwhile.

I rebased my change and fixed merge conflicts.

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

Adjust the header names so the regular headers and the pycore headers appear in the same order makes sense to me.

@ncoghlan
Copy link
Contributor

Oh, you shouldn't skip the NEWS entry though - NEWS entries are also for folks building from source, and moving header files around can absolutely break third party build scripts.

@vstinner
Copy link
Member Author

Oh, you shouldn't skip the NEWS entry though - NEWS entries are also for folks building from source, and moving header files around can absolutely break third party build scripts.

Include/internal/ only contains code for Py_BUILD_CORE. This API is strictly private and so can be broken anytime.

Do you see anything in Include/internal/ which is not... internal?

@vstinner
Copy link
Member Author

third party build scripts

I don't understand what you mean here.

@vstinner
Copy link
Member Author

vstinner commented Nov 12, 2018

me:

Include/internal/ only contains code for Py_BUILD_CORE. This API is strictly private and so can be broken anytime.

@ncoghlan: I added again "skip news". Please tell me if I misunderstood you. Do you really want to describe internal changes in the Changelog?

Benjamin Peterson documented "bpo-32264: Moved the pygetopt.h header into internal/, since it has no public APIs." This change impacts the "public API", so I agree that it deserves a NEWS entry. But here, I only modify the private internal API.

@vstinner vstinner merged commit 621cebe into python:master Nov 12, 2018
@vstinner vstinner deleted the rename_internal branch November 12, 2018 15:53
@vstinner
Copy link
Member Author

Thanks for your review @ncoghlan!

@ncoghlan
Copy link
Contributor

@vstinner For folks consuming source archives (Linux distros, anyone embedding Python in a larger application, etc) rather than prebuilt binaries, the build process is something that can break, even for Py_BUILD_CORE only changes.

For those folks, when they pull a new source archive, their builds may break, especially if they're applying additional downstream patches the way Linux distros do.

The NEWS entry for this header file rearrangement should go in the Build section (https://github.com/python/cpython/tree/master/Misc/NEWS.d/next/Build) rather than the C API section (since these headers aren't part of the public API at all), but it should still exist.

@vstinner
Copy link
Member Author

@ncoghlan: I wrote PR #10666 to add a single NEWS entry for the whole bpo.

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.

5 participants