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

Patching out dependencies on windows should have gotten easier with 3.12 #632

Open
h-vetinari opened this issue Jun 22, 2023 · 0 comments
Open

Comments

@h-vetinari
Copy link
Member

There's a larger discussion that was started about build system changes/improvements in CPython.

In it, Steve Dower brought up one possible improvement:

Easier way to constrain external inputs/libraries/dependencies (it’s easy enough on Windows, but if I want to make relocatable POSIX builds or use my own copies of various libraries rather than the system ones, it’s a real pain - I figured it out once, but then something else changed and broke it all on me).

Since I couldn't respond in that thread (only CPython core developers can), I wrote Steve directly:

To modify this slightly: just being able to point to other libraries rather than the vendored ones would be a huge benefit for distributors, who often want to build against their own openssl/sqlite/bzip/zlib etc. I also have to respectfully disagree that this is "easy" on windows: It took a long time to do this right in conda-forge, it's a mess of patches, and modifying the CRLF-wrapped XMLs with patch (infra was built to deal with more VCS's than just git) is really painful.

He kindly responded (I asked for permission to share):

It got easier recently because I made changes to make it easier (and would’ve done them sooner if I’d heard about this feedback) 🙂 But I needed it for myself, so I did it.

Check out the changes in python/cpython@f6c53b8

The basic idea is you generate a whole .props file containing the directories (and for Tkinter, I think you need the version numbers too) and set the ExternalProps property/envvar to reference it. I’m using this now to build my internal set of libraries in one build (and generate the props file in that) and then inject them into other builds later as a big blob. The paths in the props file can use the $(MSBuildThisFileDirectory) variable to do relative paths from wherever it’s loaded from, or other environment variables or whatever you like.

My response (lightly edited for clarity):

Ah, yeah, I'm not up to speed yet with what's in 3.12 -- exciting, thanks!

I see the issue where this is documented. Still, would you mind if I copy the bulk of your message here to our issue tracker? (I can try to paraphrase it of course, if you prefer)

Also cool that this was backported, I see that this already got partially reflected in 5f32669. Looking at the changes there, we still need (citation needed; it's possible/likely that this can be improved) to do things like adapt the include directories or switch off static builds:

-    <IncludeExternals Condition="$(IncludeExternals) == '' and Exists('$(zlibDir)\zlib.h')">true</IncludeExternals>
+    <IncludeExternals Condition="$(IncludeExternals) == '' and Exists('$(condaDir)\include\zlib.h')">true</IncludeExternals>
-      <AdditionalIncludeDirectories>$(lzmaDir)src/liblzma/api;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
-      <PreprocessorDefinitions>WIN32;_FILE_OFFSET_BITS=64;_CRT_SECURE_NO_DEPRECATE;_CRT_NONSTDC_NO_DEPRECATE;LZMA_API_STATIC;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <AdditionalIncludeDirectories>$(condaDir)\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <PreprocessorDefinitions>WIN32;_FILE_OFFSET_BITS=64;_CRT_SECURE_NO_DEPRECATE;_CRT_NONSTDC_NO_DEPRECATE;%(PreprocessorDefinitions)</PreprocessorDefinitions>

His response:

Yeah, feel free.

All of those changes can be added to the new props file. The IncludeExternals property is only set if it’s empty, so if you set it to true first then the later logic is ignored. And the ItemDefinitionGroup//ClCompile items are additive, so you can specify them in your own file as well. Missing include directories are ignored (though it may be a potential search path hijack if you don’t set lzmaDir), and you should be able to add LZMA_API_STATIC to UndefinePreprocessorDefinitions to omit it.

I’d also merge a change to split that variable out into its own property. So if IncludeLzmaStatic is empty or “true” then we set it, otherwise leave it out, so it’s easy for you to set it to “false”.

In short:

  • we should be able to simplify the patching process for 3.12 a little by generating a .props file that sets IncludeExternals (to avoid patching the include paths)
  • other things like removing LZMA_API_STATIC should be possible as well, either through UndefinePreprocessorDefinitions or by helping upstream the proposed IncludeLzmaStatic
  • long-term we might get a better build system setup (if there's interest, I'm sure there's a constructive way to provide feedback to that discourse thread)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant