Skip to content
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

Relative include path of generated frozen module in getpath.c #113039

Closed
itamaro opened this issue Dec 13, 2023 · 8 comments
Closed

Relative include path of generated frozen module in getpath.c #113039

itamaro opened this issue Dec 13, 2023 · 8 comments
Labels
build The build process and cross-build type-feature A feature request or enhancement

Comments

@itamaro
Copy link
Contributor

itamaro commented Dec 13, 2023

Feature or enhancement

Proposal:

in getpath.c, the generated frozen getpath.h is [included using relative path](

#include "../Python/frozen_modules/getpath.h"

I'd like to consider changing this to use absolute include path.
my primary motivation is to make it possible to build python using alternative build systems (buck2 in my case), where frozen modules are generated as intermediate build artifacts, and it's not straight forward (read: I couldn't figure it out yet) to make the generated header "look like" it's in that relative path.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Linked PRs

@itamaro itamaro added type-feature A feature request or enhancement build The build process and cross-build labels Dec 13, 2023
@itamaro
Copy link
Contributor Author

itamaro commented Dec 13, 2023

I verified that using absolute path works for in-tree and out-of-tree builds on MacOS and Linux.

CI on gh-113022 indicates that Windows doesn't like it, but I don't have access to a Windows machine to debug. @zooba do you think this would be an easy fix on Windows?

@zooba
Copy link
Member

zooba commented Dec 13, 2023

I'd rather refer to it as just #include "getpath.h" and provide an extra include directory to that file when it's being built (see PCbuild/pythoncore.vcxproj which has a few file-specific PreprocessorDefinitions elements - you can add a file-specific AdditionalIncludeDirectories in this case).

I'm not sure where the relative path came from, but I don't like it. I'm also not a huge fan of generated files going back into the source tree (I prefer them in $(IntDir) so that read-only source trees are supported), but that seems a big and unnecessary change right now.

@erlend-aasland
Copy link
Contributor

[...] I'm also not a huge fan of generated files going back into the source tree (I prefer them in $(IntDir) so that read-only source trees are supported), but that seems a big and unnecessary change right now.

+1 to all you said, but especially this part. We should create an issue for this (unless there already is one).

@itamaro
Copy link
Contributor Author

itamaro commented Dec 15, 2023

I'm also not a huge fan of generated files going back into the source tree

completely agree about the source tree. on Linux it seems this is already the case (in OOT builds, the frozen headers go in the build tree).
does Windows support OOT builds? how would I set this up if I wanted to test?

I'd rather refer to it as just #include "getpath.h" and provide an extra include directory to that file when it's being built

I think I'd prefer seeing the full path there -- seeing just getpath.h would be surprising ("where am I supposed to see this file on disk?") and probably require a comment pointing at the Python/frozen_modules directory - so why not have that bit in the include?

in any case, I was able to cheat and get CI on gh-113022 passing by keeping the relative include only on Windows.
I can try messing with AdditionalIncludeDirectories and see if I can make it work - might take a while as I am quite clueless about Windows :)

@itamaro
Copy link
Contributor Author

itamaro commented Dec 17, 2023

figured out AdditionalIncludeDirectories and published the PR

@zooba
Copy link
Member

zooba commented Dec 18, 2023

does Windows support OOT builds? how would I set this up if I wanted to test?

It's already the default - build files go into PCbuild/obj and outputs into PCbuild/amd64 (or win32/arm64). You can set Py_IntDir and/or Py_OutDir environment variables before building in order to move them.

I think I'd prefer seeing the full path there -- seeing just getpath.h would be surprising ("where am I supposed to see this file on disk?") and probably require a comment pointing at the Python/frozen_modules directory - so why not have that bit in the include?

Because it's still going to be relative to an implied include dir, it's just going to make it harder to guess which one because the file is so much further away from any of the directories.

Adding the comment is fine. You can give a better explanation that way.

@zooba
Copy link
Member

zooba commented Dec 18, 2023

The PR was fine though, so I merged it. If you want to do more work to avoid writing files back into the source tree at build time, feel free, but I didn't see any reason to hold up that first change.

@itamaro
Copy link
Contributor Author

itamaro commented Dec 18, 2023

Thanks @zooba !

I'll close this issue now, as the relative include issue has been resolved. I opened gh-113258 to follow-up on moving the frozen modules out of the source tree.

@itamaro itamaro closed this as completed Dec 18, 2023
ryan-duve pushed a commit to ryan-duve/cpython that referenced this issue Dec 26, 2023
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants