Add config options for build and python versions. Add configs for windows and linux arm64.#38
Add config options for build and python versions. Add configs for windows and linux arm64.#38Ivorforce wants to merge 1 commit intomaiself:masterfrom
Conversation
Ivorforce
force-pushed
the
choose-python-scons
branch
from
5f1183a to
d0fbdcf
Compare
|
I tried adding windows and linux arm64 builds, but they are failing because of binary incompatibility: That's a ToDo for the future. Arm runners are currently an Enterprise feature. The strip problem may be solvable with more infrastructure: https://github.com/electron/electron/blob/main/script/strip-binaries.py#L16-L25 But then, we also run |
Ivorforce
force-pushed
the
choose-python-scons
branch
2 times, most recently
from
b247007 to
13e116a
Compare
maiself
added
enhancement
Ivorforce
force-pushed
the
choose-python-scons
branch
from
13e116a to
bc7f14a
Compare
maiself
left a comment
maiself
left a comment
There was a problem hiding this comment.
A lot to look at, will have to do a deeper look later.
| auto py_minor = std::to_string(PY_MINOR_VERSION); | ||
| auto py_version = py_major + "." + py_minor; | ||
| auto py_version_no_dot = py_major + py_minor; | ||
| auto python_zip_name = "python" + py_version_no_dot + ".zip"; |
There was a problem hiding this comment.
Naming of the zip here is pretty standard. I believe there is a document in the python manual detailing the layout, but don't have a link on hand.
There was a problem hiding this comment.
It's been a while so I don't remember my reason for changing it. I think it was because I wanted the zip name to be consistent with the lib name, which uses dots (e.g. python3.2). That makes more sense to me, but I suppose if there's a PEP for a python lib zip name, I agree we should adhere to it.
Ivorforce
force-pushed
the
choose-python-scons
branch
from
bc7f14a to
c91852d
Compare
| env['python'] = prepare_python.prepare_for_platform(env['platform'], env['arch'], | ||
| src_dir = src, dest_dir = dest) | ||
|
|
||
| prepare_python_alias = env.Alias("prepare_python", [ |
There was a problem hiding this comment.
Why was the alias removed? I think the alias may have existed to ensure something else built or for some build optimization (don't remember exactly)
May need to review this part again once I understand what's happening better.
There was a problem hiding this comment.
It's been a while, but IIRC the Command usage did pretty much the same as Builder(Action) which is in use right now, but more concisely and with less call overhead. I didn't find a reason why it was implemented with aliases and builders.
|
|
||
|
|
||
| def _append_python_config(env, target, **kwargs): | ||
| src_dir = generated_path / 'python' / prepared_python_config.name |
There was a problem hiding this comment.
Note to self: became overwhelmed by above, once understand above better review from here to end of file.
| class PlatformConfig: | ||
| platform: str | ||
| arch: str | ||
| python_version_major: str |
There was a problem hiding this comment.
Naming of these is confusing me, would like to come up with something more concise as well as descriptive.
There was a problem hiding this comment.
You mean the members? I'm open to any changes :)
If you're referring to the python version names specifically, those are named from semver's major-minor-patch system.
|
|
||
| shutil.make_archive(dest_dir / f'python{config.python_version_minor}-lib', 'zip', root_dir=src / config.python_lib_dir, base_dir='') | ||
|
|
||
| if __name__ == '__main__': |
There was a problem hiding this comment.
I might want to add this back but that can be a separate patch for later
There was a problem hiding this comment.
It didn't really make sense to me anymore because the new system allows for thousands of configs (based on indygreg version numbers), instead of just a handful like before.
…ell as python version. Move build files. Unlock windows and linux build configs for arm64.
Ivorforce
force-pushed
the
choose-python-scons
branch
from
c91852d to
1747cf6
Compare
2 participants
This is quite a big change. Review required. I haven't tested running this with different python versions yet, will do this later.
Note (I think) the
"python"option available previously didn't do anything, as the environment variable was overridden anyway.