conda-forge / conda-forge/python-feedstock

Patching out dependencies on windows should have gotten easier with 3.12

Aperta
#632 0 commenti 1 reazione 0 assegnatari Vedi su GitHub
Lingua principale
Shell
Stelle
47
Fork
112
Merge medio
2g 9h
PR unite (30g)
18

Descrizione

There's a larger [discussion](https://discuss.python.org/t/what-do-you-want-to-see-in-tomorrow-s-cpython-build-system/28197) 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](https://github.com/conda-forge/python-feedstock/tree/main/recipe/patches) 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](https://github.com/conda-forge/python-feedstock/blob/main/recipe/patches/make-mixed-crlf-patch.py).

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 https://github.com/python/cpython/commit/f6c53b80a16f63825479c5ca0f8a5e2829c3f505
>
> 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](https://github.com/python/cpython/issues/101522) 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 https://github.com/conda-forge/python-feedstock/commit/5f32669ae99392185da69f5b04d1c45046bedca1. 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:
>
>
> ```diff
> - true
> + true
> ```
>
> ```diff
> - $(lzmaDir)src/liblzma/api;%(AdditionalIncludeDirectories)
> - WIN32;_FILE_OFFSET_BITS=64;_CRT_SECURE_NO_DEPRECATE;_CRT_NONSTDC_NO_DEPRECATE;LZMA_API_STATIC;%(PreprocessorDefinitions)
> + $(condaDir)\include;%(AdditionalIncludeDirectories)
> + WIN32;_FILE_OFFSET_BITS=64;_CRT_SECURE_NO_DEPRECATE;_CRT_NONSTDC_NO_DEPRECATE;%(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)

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.