Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable building for Windows ARM64 on a non-ARM64 device #1144
Enable building for Windows ARM64 on a non-ARM64 device #1144
Changes from 4 commits
ea559af
310cfda
6bcca2d
e18c21a
dc14bd1
0704190
222d56b
36442d5
b5b8e12
71df33d
59febc1
a2386a2
94a053e
9405d82
4c73245
c3dcb07
46cc262
15288be
5f391d6
634a3f7
f5c82fc
09e08f6
b8ac98e
dcc5c00
9e07dae
9a4e8f6
f277099
7b0ac9e
b94aceb
4242fe9
9dc5512
44ebbc4
1d7054c
ff2d9de
5030c2c
9815b9c
10a4e30
4531370
063b8e4
b8e4789
7fd4a22
7a9bd57
ea1d15b
18ced3c
ae58080
aa79b6e
9128465
afdae3f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
On this method more broadly, I do worry that it would end up modifying something outside of the build venv if setuptools isn't installed there, but is in a venv higher up.
So I might propose a change to this logic, like
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.
The problem with this new logic is that
call(["python" ...
doesn't actually searchPATH
to resolvepython
. It first looks in the current executable directory (normal Windows behaviour, assuming that you want the files you installed with your own app before searching other installs). So it only works if we're already running in the correct Python, in which case we may as well go straight tosys.prefix
.Also, if we're going to run Python and import it, I'd rather
import distutils; print(distutils.__file__)
and work from that to save a few assumptions. Or deliberately keep the venv path around from earlier - I tried that briefly and decided it wasn't worth the changes, but it's going to be the most reliable.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.
Oh, and we alse know that
env["PATH"]
contains the right venv first because we just updated it ourselves.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.
馃く whoa really? when you say the 'current executable directory', do you mean the working directory, which would be the project dir in this case? We do
call(["python" ...
all over the place and we've never had an issue with the wrong one being invoked. Maybe because people don't tend to have apython.exe
in the root of their project.I'd prefer that, I think. We probably won't preinstall setuptools in the build venv forever. Though I think it should be
import setuptools._distutils; print(setuptools._distutils.__file__)
, right? (though, depending on the outcome of the isolated build-backend venv conversation, it might be a moot point...)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.
No, it's more like (don't reuse this code)
os.path.split(sys.orig_argv[0])
. The current working directory lookup is a "feature" of the old cmd shell, but looking in the application directory is a "feature" of CreateProcess (the underlying OS API call). Best way to disable it is to pass a full path.That isn't forward compatible though.
setuptools._distutils
is an internal name, and I expect setuptools to deprecate theirdistutils
shim almost immediately and get people using their commands directly.But yeah, isolated backends are going to break this approach anyway. Setuptools is currently broken anyway because of another change, so if they're amenable to having their
build*
commands pick up default values from environment variables, we can probably get that in (though I'm about to be AFK for at least a week, and then at EuroPython the week after that, so it might have to wait until the sprints).