-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Copy libs/pythonXY.lib to support building of C extensions without distutils #1023
Conversation
Is there any interest on merging this PR? Pinging @dstufft as one of the project moderator recently active. |
@macisamuele Thanks for your help It would indeed be nice to get this integrated. |
I've spent the last few days experimenting in building rust from a venv with no success, I've applied the patch from this PR on the current master and it worked on the first try 🎉 |
Great 👍 Could you review this topic then: |
@jcfr I can confirm. The build with your new branch worked as expected |
c96b157
to
895742d
Compare
Thanks @macisamuele , topic updated 😄 |
2b3f876
to
9dfcb43
Compare
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 will require a rebase on master, and some test to validate the changes. Thanks!
Closing due to inactivity. |
@gaborbernat I would appreciate if you could give this a little more time. Thanks for your patience. |
9354332
to
121cc1e
Compare
Topic has been rebased and tested locally. I confirm that the
@gaborbernat I was thinking to update the test test_install_python_bin ? What do you think ? |
…stutils This issue was discovered while using virtualenv to create an isolated environment and test that a source distribution would compile without using the knowledge hardcoded in distutils.sysconfig and virtualenv/virtualenv_embedded/distutils-init.py In other word, without relying on the fact virtualenv monkey patch distutils to ensure the value associated with LIBDIR variable is correct. This is relevant because the library location is implicitly specified in PC/pyconfig.h 8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<--- [...] /* For an MSVC DLL, we can nominate the .lib files used by extensions */ #ifdef MS_COREDLL # ifndef Py_BUILD_CORE /* not building the core - must be an ext */ # if defined(_MSC_VER) /* So MSVC users need not specify the .lib file in their Makefile (other compilers are generally taken care of by distutils.) */ # if defined(_DEBUG) # pragma comment(lib,"python35_d.lib") # elif defined(Py_LIMITED_API) # pragma comment(lib,"python3.lib") # else # pragma comment(lib,"python35.lib") # endif /* _DEBUG */ # endif /* _MSC_VER */ # endif /* Py_BUILD_CORE */ #endif /* MS_COREDLL */ [...] 8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<--- References: * https://docs.python.org/2/extending/windows.html#using-dlls-in-practice * https://docs.python.org/3.6/extending/windows.html#using-dlls-in-practice Tested-by: Samuele Maci <macisamuele@gmail.com>
121cc1e
to
508919a
Compare
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.
As far as modifying that test to assert against it, I'm good with it.
# considering that on windows, python import libraries are located in | ||
# the "<root>/libs" directory, the following code will look for and | ||
# copy "pythonXY.lib". | ||
pythonlib_name = "python{}{}.lib".format(sys.version_info[0], sys.version_info[1]) |
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.
slight name tweak, python_lib_name
(the logic applies to all other names too)
pythonlib_dest_dir = os.path.join(lib_dir, "..", "libs") | ||
pythonlib_dest = os.path.join(pythonlib_dest_dir, pythonlib_name) | ||
if os.path.exists(pythonlib): | ||
logger.info("Also created %s" % pythonlib_name) |
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 not materialize the string yet, leave it extra arg to logger.
@@ -1395,6 +1395,20 @@ def install_python(home_dir, lib_dir, inc_dir, bin_dir, site_packages, clear, sy | |||
elif os.path.exists(python_dll_d_dest): | |||
logger.info("Removed %s as the source does not exist", python_dll_d_dest) | |||
os.unlink(python_dll_d_dest) | |||
|
|||
if not IS_PYPY: |
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.
Why just pypy, what about jython? We should also mention why this is so special
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.
- Jython is java-based and doesn't provide import libraries like CPython does.
- Pypy indeed provides a
libs
folder with the correspondingpythonXY.lib
file, I will update the patch to not exclude it.
# considering that on windows, python import libraries are located in | ||
# the "<root>/libs" directory, the following code will look for and | ||
# copy "pythonXY.lib". | ||
pythonlib_name = "python{}{}.lib".format(sys.version_info[0], sys.version_info[1]) |
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.
is it lib on Windows too?
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.
.lib
is indeed the extension on Windows. Test above was performed on windows and the directory were searched running find
and grep
from Git Bash.
# copy "pythonXY.lib". | ||
pythonlib_name = "python{}{}.lib".format(sys.version_info[0], sys.version_info[1]) | ||
pythonlib = os.path.join(os.path.dirname(sys.executable), "libs", pythonlib_name) | ||
pythonlib_dest_dir = os.path.join(lib_dir, "..", "libs") |
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.
Shouldn't this take the lib_dir value from
Line 1095 in 508919a
lib_dir = join(home_dir, "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.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Just add a comment if you want to keep it open. Thank you for your contributions. |
Closing due to no action on the PR for a long time, if anyone has the bandwidth to pick this up, let me know, and we can have a talk what would be a good solution here. |
Hi @gaborbernat , Thanks for the update. If I find some time, I will rebase, test locally and answer follow-up questions. Best, |
I'd like to validate this on the new rewrite branch to be honest, though not sure is advanced enough to add this. |
created #1448 to track this |
This issue was discovered while using virtualenv to create an
isolated environment and test that a source distribution would compile
without using the knowledge hardcoded in
distutils.sysconfig
andvirtualenv/virtualenv_embedded/distutils-init.py
In other word, without relying on the fact virtualenv monkey patch distutils
to ensure the value associated with
LIBDIR
variable is correct.This is relevant because the library location is implicitly specified
in PC/pyconfig.h
References: