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

Allow sysconfig to override install schemes... #23

Merged
merged 17 commits into from
Nov 13, 2021
Merged

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Jan 23, 2021

...and provide some compatibility overrides for debian and fedora until they patch sysconfig.

Fixes #12.

…ility overrides for debian and fedora until they patch sysconfig.
@jaraco
Copy link
Member Author

jaraco commented Jan 23, 2021

@encukou, @doko42 Please have a look and let me know your thoughts, especially about the broad strokes. In particular:

  • Should distutils attempt to detect Debian/Fedora/other in order to provide compatibility for systems without sysconfig overrides?
  • Does this approach satisfy the goals of customizing locations?
  • (Debian) Is there a way to make this work without the --install-layout parameter?

This change doesn't attempt to address the alternate naming for 'egg-info' files (without Python version). That'll be handled in a separate PR if needed.

@encukou
Copy link
Contributor

encukou commented Jan 26, 2021

Should distutils attempt to detect Debian/Fedora/other in order to provide compatibility for systems without sysconfig overrides?

I don't think it should. Who konws how many variants there are; Distutils isn't likely to get all of them right.
As written this hardcodes some choices even for systems with sysconfig overrides. The special-casing should only be done if sysconfig doesn't have INSTALL_SCHEMES.

Does this approach satisfy the goals of customizing locations?

For customizing locations on future systems, this looks good. Thank you!
However making 'unix_local' the default everywhere is not right: if Python itself is installed to /usr/local (default when built from source), distutils will now install packages to /usr/local/local. The /local should only be added for the system-installed Python.

Another thing I'm worried about is that distutils.command.install.INSTALL_SCHEMES ended up being API. If anyone tries to use the version here they'll get incomplete/wrong data unless _load_schemes happened to be called already.
Would it be better to move _load_schemes to module level and call it on import?
(The part in _load_schemes where self.install_layout influences global state looks suspicious; moving to a top-level function should help clear that up as well.)

distutils/command/install.py Outdated Show resolved Hide resolved
distutils/command/install.py Outdated Show resolved Hide resolved
@encukou
Copy link
Contributor

encukou commented Jan 26, 2021

cc @frenzymadness

@doko42
Copy link
Contributor

doko42 commented Jan 29, 2021

I share the intent to help users not to brick their system, having similar patches in Debian and Ubuntu.

The proposed solution however makes 'unix_local' use the same location as somebody doing

./configure --prefix=/usr/local.

Note that --prefix=/usr/local is implicit ( the default). You would now have a common modules dir for two different Python installations. I remember @warsaw suggested to use different locations for that.

@jaraco
Copy link
Member Author

jaraco commented Sep 21, 2021

As written this hardcodes some choices even for systems with sysconfig overrides. The special-casing should only be done if sysconfig doesn't have INSTALL_SCHEMES.

The _load_schemes does short-circuit if it's able to update the schemes from sysconfig. Can you propose what special casing you would like to avoid additionally?

Another thing I'm worried about is that distutils.command.install.INSTALL_SCHEMES ended up being API. If anyone tries to use the version here they'll get incomplete/wrong data unless _load_schemes happened to be called already.
Would it be better to move _load_schemes to module level and call it on import?
(The part in _load_schemes where self.install_layout influences global state looks suspicious; moving to a top-level function should help clear that up as well.)

I'd like to avoid both "behavior on import" and also help limit use of INSTALL_SCHEMES as API. I agree, changing a global variable seems wrong. In 968d298, I refactor to make the loading of install schemes still lazy, and avoid mutating the global state.

@jaraco
Copy link
Member Author

jaraco commented Sep 21, 2021

The /local should only be added for the system-installed Python.

I share the intent to help users not to brick their system, having similar patches in Debian and Ubuntu.

The proposed solution however makes 'unix_local' use the same location as somebody doing

./configure --prefix=/usr/local.

Note that --prefix=/usr/local is implicit ( the default). You would now have a common modules dir for two different Python installations. I remember @warsaw suggested to use different locations for that.

I'm unsure what logic is needed to inject local under what circumstances. I'd really like someone with more experience in this domain to propose a solution. In da44ac0, I remove the 'unix_local' install scheme for now.

@jaraco jaraco force-pushed the feature/local-schemes branch from da0e19a to dcafd57 Compare September 21, 2021 18:55
@FFY00
Copy link
Member

FFY00 commented Sep 21, 2021

IMO, having a unix_local scheme is incorrect. The Python installation should be self-contained to its prefix and not hijack the /usr/local prefix, which should be reserved for its own Python installation.

@jaraco
Copy link
Member Author

jaraco commented Sep 26, 2021

This latest draft is minimally invasive, but promises to provide the necessary hook to enable install schemes to be overridden in sysconfig. @FFY00 Can you confirm this approach uses the correct attribute?

@jaraco jaraco marked this pull request as ready for review September 26, 2021 22:17
distutils/command/install.py Outdated Show resolved Hide resolved
@jaraco jaraco force-pushed the feature/local-schemes branch from 5c128af to 28e6439 Compare November 6, 2021 15:14
@jaraco
Copy link
Member Author

jaraco commented Nov 6, 2021

Now this change incorporates the recommendation to use sysconfig.get_paths, and adopting this change causes all of the install schemes from sysconfig to be used, which would be fine except that the install schemes there have different semantics than the ones defined here. I found that incorporating the installed_base from sysconfig.get_config_vars satisfies the missing semantics, but I'm not super confident the change is stable.

The tests were only failing on Windows, which led me to realize that it's the keys also that are different between sysconfig and distutils. Distutils uses "unix" whereas sysconfig uses "posix", which means that with the current implementation, only "nt" is getting the new semantics treatment.

I think the best thing would be to also move distutils over to the "posix" naming to consistently apply the shift to sysconfig-based configs... and in fact probably remove any local schemes that are superseded by those in stdilb.

@jaraco
Copy link
Member Author

jaraco commented Nov 7, 2021

In this latest commit (5c5aed1), I update the naming to match that of upstream, and when I do, one test in particular starts to fail:

___________________________________________________________ InstallTestCase.test_home_installation_scheme ___________________________________________________________

self = <distutils.tests.test_install.InstallTestCase testMethod=test_home_installation_scheme>

    def test_home_installation_scheme(self):
        # This ensure two things:
        # - that --home generates the desired set of directory names
        # - test --home is supported on all platforms
        builddir = self.mkdtemp()
        destination = os.path.join(builddir, "installation")
    
        dist = Distribution({"name": "foopkg"})
        # script_name need not exist, it just need to be initialized
        dist.script_name = os.path.join(builddir, "setup.py")
        dist.command_obj["build"] = support.DummyCommand(
            build_base=builddir,
            build_lib=os.path.join(builddir, "lib"),
            )
    
        cmd = install(dist)
        cmd.home = destination
        cmd.ensure_finalized()
    
        self.assertEqual(cmd.install_base, destination)
        self.assertEqual(cmd.install_platbase, destination)
    
        def check_path(got, expected):
            got = os.path.normpath(got)
            expected = os.path.normpath(expected)
            self.assertEqual(got, expected)
    
        libdir = os.path.join(destination, "lib", "python")
        check_path(cmd.install_lib, libdir)
        _platlibdir = getattr(sys, "platlibdir", "lib")
        platlibdir = os.path.join(destination, _platlibdir, "python")
        check_path(cmd.install_platlib, platlibdir)
        check_path(cmd.install_purelib, libdir)
>       check_path(cmd.install_headers,
                   os.path.join(destination, "include", "python", "foopkg"))

distutils/tests/test_install.py:65: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
distutils/tests/test_install.py:57: in check_path
    self.assertEqual(got, expected)
E   AssertionError: '/Library/Frameworks/Python.framework/Versi[19 chars]thon' != '/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr00[49 chars]opkg'
E   - /Library/Frameworks/Python.framework/Versions/3.10/include/python
E   + /var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/tmp02xwxxw4/installation/include/python/foopkg

This failure reveals several defects with the current approach:

  • distutils code is poorly covered. If only this test about "home installation" is failing, it illustrates that the more common installation schemes, where at least part of this issue (include/headers checks) exists also, aren't failing so aren't covered.
  • the upstream implementation doesn't include {dist_name} at the end of the include paths.
  • even the base paths don't match.

I believe this all boils down to detecting that posix_home.include is very different for distutils and sysconfig:

distutils: {base}/include/{implementation_lower}/{dist_name}
sysconfig: '{installed_base}/include/python'

Ignoring for a moment that {implementation_lower} will be lost (that was added very recently in #58), distutils already has very different expectations about that config value.

At this point, I'm not sure it's going to be possible to rely on install schemes from sysconfig. They've drifted too far from what distutils expects to be reliable.

I can imagine that maybe distutils could keep its own copy of the default sysconfig values and the more customized distutils values, and prefer the customized values if the sysconfig values are set to defaults. But what a nightmare would that be?

Until the install schemes specified in sysconfig are aligned with those that distutils expects, this approach is not going to be viable.

It seems to me we have two choices:

  • come up with another mechanism to allow downstream vendors and python implementations to customize installation schemes.
  • formalize the install schemes schema to align with distutils' needs

@FFY00 Are those our only choices? How would you recommend to proceed?

@FFY00
Copy link
Member

FFY00 commented Nov 7, 2021

Yes, that was the main issue I found when migrating from distutils. It is described here: https://bugs.python.org/issue44445

Currently, I am getting the headers path from distutils and adding it to the scheme dict.
https://github.com/FFY00/installer/blob/ec29913235b2e4dd5ee6a75e38e6302355df8128/src/installer/__main__.py#L118-L128

I think the best way to solve this is, like proposed in the bpo, adding a site-include scheme key and having distutils append the distname to it for the headers path.

This is a major issue with the distutils deprecation and must be solved before 3.12. If distutils cannot use sysconfig for the paths, there is an incompatibility and sysconfig is not a proper replacement.

@jaraco
Copy link
Member Author

jaraco commented Nov 12, 2021

Thanks for the pointers there. I do indeed see the issue. I do believe it's going to be a long and difficult road to harmonize include and headers, so my instinct here is to do the following:

  • let distutils still depend on headers
  • because headers aren't defined in sysconfig, merge the schemes such that keys missing from schemes in sysconfig can be supplied by keys from schemes in distutils.

This approach should provide compatibility while also allowing the downstream packagers to override the headers value.

@FFY00
Copy link
Member

FFY00 commented Nov 12, 2021

This sounds reasonable to me as a solution for now, until we fix this properly.

@jaraco
Copy link
Member Author

jaraco commented Nov 13, 2021

Tests are passing here and also in pypa/setuptools#2875.

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.

Allow customization of locations
5 participants