-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
gh-138489: Install build-details.json
on Windows
#138490
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
base: main
Are you sure you want to change the base?
Conversation
<Exec Command='setlocal | ||
set PYTHONPATH=$(PySourcePath)Lib |
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.
Unsure if these bits are needed, I copied them from the entry slightly further down.
* KeyError is not raised for defaultdict * Fix relative paths on different drives on Windows
data['language']['version_info'] = version_info_to_dict(sys.version_info) | ||
|
||
data['implementation'] = vars(sys.implementation) | ||
data['implementation'] = vars(sys.implementation).copy() |
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 needed, currently the script overwrites sys.implementation.version
in the running interpreter.
I'll be honest, I haven't looked at the files these generate. For Windows, I'd expect them to be basically static - installs are entirely portable and all options are defined in our repo (though some can be overridden at build time, so picking them up later is okay). Is that the case here? We likely also need changes to the |
Example build-details.json (expand){
"schema_version": "1.0",
"base_prefix": "C:\\path\\to\\cpython",
"base_interpreter": "C:\\path\\to\\cpython\\Scripts\\python315",
"platform": "win-amd64",
"language": {
"version": "3.15",
"version_info": {
"major": 3,
"minor": 15,
"micro": 0,
"releaselevel": "alpha",
"serial": 0
}
},
"implementation": {
"name": "cpython",
"cache_tag": "cpython-315",
"version": {
"major": 3,
"minor": 15,
"micro": 0,
"releaselevel": "alpha",
"serial": 0
},
"hexversion": 51314848,
"supports_isolated_interpreters": true
},
"abi": {
"flags": [],
"extension_suffix": "_d.cp315-win_amd64.pyd"
},
"suffixes": {
"source": [
".py",
".pyw"
],
"bytecode": [
".pyc"
],
"extensions": [
"_d.cp315-win_amd64.pyd",
"_d.pyd"
]
},
"libpython": {
"dynamic": "C:\\path\\to\\cpython\\libs\\python315_d.dll",
"link_extensions": false
},
"c_api": {
"headers": "C:\\path\\to\\cpython\\Include"
}
} |
Yeah, that's what I thought. It's okay for one you've built in place, but we shouldn't include any absolute paths in a "real" build (and nothing relative to "outside" the prefix). |
@zooba there is a The remaining issue would be the example with relative paths{
"schema_version": "1.0",
"base_prefix": "C:\\path\\to\\cpython",
"base_interpreter": ".\\Scripts\\python315",
"platform": "win-amd64",
"language": {
"version": "3.15",
"version_info": {
"major": 3,
"minor": 15,
"micro": 0,
"releaselevel": "alpha",
"serial": 0
}
},
"implementation": {
"name": "cpython",
"cache_tag": "cpython-315",
"version": {
"major": 3,
"minor": 15,
"micro": 0,
"releaselevel": "alpha",
"serial": 0
},
"hexversion": 51314848,
"supports_isolated_interpreters": true
},
"abi": {
"flags": [],
"extension_suffix": "_d.cp315-win_amd64.pyd"
},
"suffixes": {
"source": [
".py",
".pyw"
],
"bytecode": [
".pyc"
],
"extensions": [
"_d.cp315-win_amd64.pyd",
"_d.pyd"
]
},
"libpython": {
"dynamic": ".\\libs\\python315_d.dll",
"link_extensions": false
},
"c_api": {
"headers": ".\\Include"
}
} A |
This shouldn't matter for real, as none of the paths should ever point outside of the prefix. For local builds it might, but a cross-drive path in that case just becomes an absolute path. And yeah, setting |
This comment was marked as outdated.
This comment was marked as outdated.
# Conflicts: # Lib/test/test_build_details.py
I've updated this, but I've had to make a few changes to cc @zooba if you have time for a review! A |
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.
Most of the change is good (based on the last 4 commits), but we're going to have to clone a lot more of the logic into PC/layout in order to handle cross-compiling/packaging.
import sys | ||
from pathlib import Path | ||
|
||
PEP739_SCHEMA_VERSION = '1.0' |
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.
PEP739_SCHEMA_VERSION = '1.0' | |
__all__ = ["write_relative_build_details"] | |
PEP739_SCHEMA_VERSION = '1.0' |
ROOT_DIR = Path( | ||
__file__, # PC/layout/support/build_details.py | ||
'..', # PC/layout/support | ||
'..', # PC/layout | ||
'..', # PC | ||
'..', # <src/install dir> | ||
).resolve() | ||
TOOLS_BUILD_DIR = ROOT_DIR / 'Tools' / 'build' | ||
|
||
sys_path = sys.path[:] | ||
try: | ||
sys.path.insert(0, str(TOOLS_BUILD_DIR)) | ||
import generate_build_details | ||
except ImportError: | ||
generate_build_details = None | ||
finally: | ||
sys.path = sys_path | ||
del sys_path |
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'm afraid this probably needs to replicate most of the functionality from that script and do trickier calculations. The repo containing the running PC/layout isn't necessarily the same as the commit used to build, and the running interpreter isn't necessarily the same as the one we just built.
So the details have to be calculated from the referenced source files (i.e. layout.support.constants
and layout.support.arch
), not from the current runtime at all.
That said, I haven't dug into the write_build_details
function and maybe it does it, but it really doesn't look like enough information is being provided to make it viable (especially when you factor in that {base_path}\python.exe
is not executable).
"flat-dlls", | ||
"underpth", | ||
"precompile", | ||
"build-details-json", |
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 suspect the contents of the file needs to be different for the embeddable package (no Lib
directory or Includes/libs, for example). Maybe just omit it for now
ROOT_DIR = Path( | ||
__file__, # PC/layout/support/build_details.py | ||
'..', # PC/layout/support | ||
'..', # PC/layout | ||
'..', # PC | ||
'..', # <src/install dir> | ||
).resolve() | ||
TOOLS_BUILD_DIR = ROOT_DIR / 'Tools' / 'build' | ||
|
||
sys_path = sys.path[:] | ||
try: | ||
sys.path.insert(0, str(TOOLS_BUILD_DIR)) | ||
import generate_build_details | ||
except ImportError: | ||
generate_build_details = None | ||
finally: | ||
sys.path = sys_path | ||
del sys_path |
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.
Do we really need to import the script here instead of invoking it?
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.
We can't do either - we might need to generate the file without being able to launch the runtime. See my comment above on the same section of code.
Closes #138489. cc @FFY00 as PEP author.
I'd appreciate a review/sanity check on the
.vcxproj
files.I've also set
abi.flags
in the JSON document to the empty list, rather than e.g. copying the newABIFLAGS
code. The PEP defines it as:A
build-details.json
is missing on some platforms #138489