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

Ensure Windows SDK directories are not cleared when caller specifies include/library dirs #153

Merged
merged 15 commits into from
Aug 10, 2022

Conversation

zooba
Copy link
Contributor

@zooba zooba commented Jun 17, 2022

This corrects a long-standing bug in the _msvccompiler implementation where the INCLUDE and LIB variables from the environment would be entirely overridden by --include-dirs and --library-dirs options (regardless of how they were passed). Instead, these should be appended after any user-specified directories. Overriding the _fix_*_args methods handles this for all users of the class.

This will make fixes such as what PyWin32 uses redundant.

@zooba
Copy link
Contributor Author

zooba commented Jun 17, 2022

Fully expecting some test breaks, btw, as it does change the inspectable values in compiler.include_dirs and compiler.library_dirs. That might be an issue for some projects, but I think overall it's a net improvement - the only alternative is monkey-patching in the value.

This change should also let us enable cross-compiling through cibuildwheel, which otherwise can't patch this at runtime.

@zooba
Copy link
Contributor Author

zooba commented Jun 27, 2022

@jaraco You know I don't like pinging you directly on stuff, but I'd love to get this off my in-progress list before I take a couple weeks away :-)

@jaraco
Copy link
Member

jaraco commented Jun 29, 2022

I welcome pings. Can I take a look at this Friday?

@jaraco
Copy link
Member

jaraco commented Jul 1, 2022

It feels like this approach is papering over a design defect. From distutils' perspective (and setuptools'), add_include_dir is used exclusively by this compiler, so it doesn't make sense to leave it lying around if its only usage was broken.

I'd rather see a solution that involves re-designing CCompiler such that add_*_dirs does the right thing. Or if add_*_dirs was never going to work, remove it and provide a proper interface where a subclass can supplement the include dirs.

More importantly, if this bug has been lying around essentially since its inception, perhaps it would make sense to create a regression test to capture the flaw (missed expectation), especially since the change didn't break any tests.

@zooba
Copy link
Contributor Author

zooba commented Jul 4, 2022

The design defect has been in MSVCCompiler ever since I rewrote it (maybe earlier, I didn't check). Fundamentally, MSVC requires all its directories specified with -I/-L, while CCompiler assumes that only extra directories need to be specified this way. I did this by finding the system directories and adding them in the user option space, when they always ought to have been kept aside.

This change handles that assumption by injecting the compilers directories later on, just before launching the compiler. They get added after any user directories, so nothing should break. At worst, the system directories will be added twice by everyone with workarounds (e.g. PyWin32). (I guess technically the worst is that some deliberately(!?) failing builds will start succeeding, but that's the point of bug fixes.)

I'd love to completely rework all the compilers, but since that would break everyone's hacks, it can't really be in setuptools or distutils ;-)

@zooba
Copy link
Contributor Author

zooba commented Jul 17, 2022

Any more thoughts on this one? I'm still not sure what a test would prove, and the path ordering should ensure that it can only affect builds that currently fail with missing header files.

@jaraco jaraco self-assigned this Jul 30, 2022
@jaraco
Copy link
Member

jaraco commented Jul 30, 2022

I notice that set_include_dirs, which is what the different commands use, purports not to affect include dirs used by the compiler by default:

def set_include_dirs(self, dirs):
"""Set the list of directories that will be searched to 'dirs' (a
list of strings). Overrides any preceding calls to
'add_include_dir()'; subsequence calls to 'add_include_dir()' add
to the list passed to 'set_include_dirs()'. This does not affect
any list of standard include directories that the compiler may
search by default.
"""
self.include_dirs = dirs[:]

@jaraco
Copy link
Member

jaraco commented Jul 31, 2022

I'm working on writing a test for this change, but I can't even find where _fix_compile_args is called for this compiler. Have you confirmed this change has the desired effect? Can you create repro steps that replicate the broken behavior?

@zooba
Copy link
Contributor Author

zooba commented Aug 1, 2022

I notice that set_include_dirs, which is what the different commands use, purports not to affect include dirs used by the compiler by default:

Yes, this is why my change makes it so that it doesn't ;-)

"By default" behaves slightly differently on Windows, because the default ones are smuggled in through the environment, not automatically determined by the compiler itself. Since we wipe out the environment, we need to pass them along ourselves, and previously were doing it through set_include_dirs essentially, when we should've been doing it independently from this.

Have you confirmed this change has the desired effect? Can you create repro steps that replicate the broken behavior?

It definitely works. I can have a go at repro steps, but essentially what I'll try to do is create a .c file that includes something from the Windows SDK, and show that calling build_ext -L <other dir> prevents the SDK from being found. Maybe you'll get to it before me.

@zooba
Copy link
Contributor Author

zooba commented Aug 1, 2022

I'm close to a repro. It requires attempting to cross-compile and specifying -p on the command line (or presumably through a configuration file), which causes build_ext to initialise the compiler earlier. Then specifying any overrides with -I will wipe the include directories.

@zooba
Copy link
Contributor Author

zooba commented Aug 1, 2022

So the repro without subclassing the command requires the -p option to be passed. Passing win32 to a 64-bit interpreter is fine, as long as it doesn't match get_platform() (the default). This causes the compiler to initialise early, and later settings will clear its includes.

Alternatively, subclassing build_ext and calling set_include_dirs, no matter how "correctly", will break it too. This is a pretty common override that I see in projects.

from setuptools import setup, Extension
from setuptools.command.build_ext import build_ext as _build_ext

EXT_MODULES = [Extension(
    'file',
    ['file.c'],
)]

class build_ext(_build_ext):
    def compile(self):
        self.set_include_dirs(self.include_dirs)
        super().compile()

setup(
    name="proj",
    version="1.0",
    ext_modules=EXT_MODULES,
    cmdclass={"build_ext": build_ext},
)

One thing that is missing from my patch that only impacts the include directories is, as you pointed out, _fix_compile_args doesn't get called. It looks like it is meant to be called from _setup_compile rather than duplicating the same code in that function. I was mainly interested in the library dirs, which is how I had all of my stuff working with this change.

Considering all the compiler classes use this for preprocessing but not compiling, and the method says it's for compiling, I assume this was an oversight and it should be changed for all of them. I'll update my PR with it for reference, but if we only want to enable it for MSVC then I'm not going to argue too strongly. Nobody else overrides it within distutils or setuptools currently.

@jaraco
Copy link
Member

jaraco commented Aug 6, 2022

Today I stumbled on #166, where one of the extension building tests for Windows is failing to link. I wonder if that test somehow serves as a repro for this issue.

Edit: No - the failure there was a bug in the test suite.

@jaraco
Copy link
Member

jaraco commented Aug 6, 2022

In the bugfix/153-test branch, I've started work on a test to capture the failure. It doesn't fail yet because:

  • The C file doesn't contain any SDK calls.
  • The set_{include,library}_dirs calls happen before .compile, which is what triggers compiler.initialize() (add add_*_dirs happens in initialize, so won't be affected by the earlier set).

I'm thinking for the second issue to simply repeat the set_*_dirs call and then run compile again. Is that what's happening with build_ext?

Steve, can you help by providing some minimal C file that one would expect to fail in this scenario? I'd be happy with one that just fails on Windows for now, but it'd be nice to have a comparable one for Unix (maybe that relies on libc?).

Feel free to send a PR to that branch, or just post it here and I'll integrate it.

@zooba
Copy link
Contributor Author

zooba commented Aug 8, 2022

Sure. I got it to fail with this c file:

#include <Python.h>
#include <windows.h>

PyMODINIT_FUNC
PyInit_file(void)
{
    return NULL;
}

This setup.py:

from setuptools import setup, Extension

setup(
    name="proj",
    version="1.0",
    ext_modules=[Extension('file', ['file.c'])],
)

And then invoking as python setup.py -p win32 (where the default plat-name would have been win-amd64).

Changing the default platform is the easiest way to trigger this, though I've seen it in more complex cases where the compiler is made to initialize early or where paths are updated later.

You won't see the same issue on other platforms because it only affects _msvccompiler, which is only used on Windows.

@jaraco
Copy link
Member

jaraco commented Aug 9, 2022

I believe I've replicated the failure in 71cffcb.

@zooba
Copy link
Contributor Author

zooba commented Aug 9, 2022

I believe I've replicated the failure in 71cffcb.

Yeah, that ought to do it. I mainly avoided doing it that way because nobody "out there" does that - they always get caught by the more subtle cases.

kxrob added a commit to kxrob/distutils that referenced this pull request Aug 20, 2022
.. including the MSVC & SDK include dirs after compiler.initialize()

Changes in pypa#153 broke compatibility, and used shadowed class
variables unnecessarily in an odd manner hiding the MSVC dirs.
kxrob added a commit to kxrob/distutils that referenced this pull request Aug 20, 2022
thus including the MSVC & SDK include dirs after compiler.initialize()

Changes in pypa#153 broke compatibility (pywin32), used shadowed class
variables unnecessarily in an odd manner and hided the MSVC dirs
from decent public usage.
kxrob added a commit to kxrob/distutils that referenced this pull request Aug 20, 2022
thus including the MSVC & SDK include dirs after compiler.initialize()

Changes in pypa#153 broke compatibility (pywin32), used shadowed class
variables unnecessarily in an odd manner and hided the MSVC dirs
from decent public usage.
kxrob added a commit to kxrob/distutils that referenced this pull request Aug 20, 2022
including the SDK include dirs after compiler.initialize()

Changes in pypa#153 broke compatibility (pywin32), used shadowed class
variables in an odd manner and hided the MSVC dirs from decent
public access.
kxrob added a commit to kxrob/distutils that referenced this pull request Aug 20, 2022
including the SDK dirs after compiler.initialize()

Changes in pypa#153 broke compatibility (pywin32), used shadowed class
variables in an odd manner and hided the MSVC dirs from public
access.
kxrob added a commit to kxrob/distutils that referenced this pull request Aug 20, 2022
including the SDK dirs after compiler.initialize()

Changes in pypa#153 broke compatibility (pywin32), used shadowed class
variables in an odd manner and conceiled the MSVC dirs from public
access.
kxrob added a commit to kxrob/distutils that referenced this pull request Aug 20, 2022
including the SDK dirs after compiler.initialize()

Changes in pypa#153 broke compatibility (pywin32), used shadowed class
variables in an odd manner and conceiled the MSVC dirs from public
access.
kxrob added a commit to kxrob/distutils that referenced this pull request Aug 20, 2022
including the SDK dirs after compiler.initialize()

Changes in pypa#153 broke compatibility (pywin32), used shadowed class
variables in an odd manner and conceiled the MSVC dirs from public
access.
kxrob added a commit to kxrob/distutils that referenced this pull request Aug 20, 2022
including the SDK dirs after compiler.initialize()

Changes in pypa#153 broke compatibility (pywin32), used shadowed class
variables in an odd manner and conceiled the MSVC dirs from public
access.
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

Successfully merging this pull request may close these issues.

3 participants