labscript-suite / labscript-utils

Shared modules used by the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦. Includes a graphical exception handler, debug tools, configuration management, cross platform filepath conversions, unit conversions and custom GUI widgets.
http://labscriptsuite.org
Other
2 stars 45 forks source link

Editable installs with setuptools>63 broken #90

Closed dihm closed 1 year ago

dihm commented 1 year ago

Between Johannes and Chris, an issue has been identified that prevents proper device path resolution when using editable installs and setuptools that is PEP 660 compliant (ie >=64).

Doing a very minor amount of sleuthing on my own, I've confirmed that PEP 660 compliant setuptools developer builds no longer call the develop command, which is what installs the labscript-suite.pth file into site-packages via a command overload in labscript-utils. For me, the called command is now editable_wheel, but this command does not appear to expose the install directory. Other called commands (like build_py) also do not seem to expose this. Furthermore, there doesn't really seem to be a great backwards compatible option, meaning whatever is done will likely need to be setuptools version dependent, which is a little annoying.

Looking over the docs, it appears that the exact method used for editable installs is not guaranteed on every system. On my system, the last method using a dynamic import finder happens to be used. Given what the docs say, this may be the consistently chosen option since we control project structure and install options, but this may be something to be aware of.

dihm commented 1 year ago

On a semi-related note, running the pip install command in verbose mode exposes a large number of deprecation warnings regarding installing packages as data. Here is an example

  C:\Users\naqsL\Miniconda3\envs\py39\lib\site-packages\setuptools\command\build_py.py:202: SetuptoolsDeprecationWarning:     Installing 'labscript_utils.excepthook' as data is deprecated, please list it in `packages`.
      !!

      ############################
      # Package would be ignored #
      ############################
      Python recognizes 'labscript_utils.excepthook' as an importable package,
      but it is not listed in the `packages` configuration of setuptools.

      'labscript_utils.excepthook' has been automatically added to the distribution only
      because it may contain data files, but this behavior is likely to change
      in future versions of setuptools (and therefore is considered deprecated).

      Please make sure that 'labscript_utils.excepthook' is included as a package by using
      the `packages` configuration field or the proper discovery methods
      (for example by using `find_namespace_packages(...)`/`find_namespace:`
      instead of `find_packages(...)`/`find:`).

      You can read more about "package discovery" and "data files" on setuptools
      documentation page.

  !!

    check.warn(importable)

This is raised for all the sub-packages of labscript-utils as well as the default_profile example folders.

chrisjbillington commented 1 year ago

Nice investigation!

Not unexpected that we don't get the install directory during the editable_wheel command, since we're building a wheel we'll need to add the .pth file to the wheel as a data file or something, not copy it to the install directory ourselves. Then it'll end up in the install directory when the wheel is subsequently installed. I'll take a look at customising that command.

We can just leave the existing custom command in as well, to support older setuptools for now - shouldn't require an explicit version check I would think.

I see the dynamic finder mechanism on my system too, so let's assume that for now and be on the lookout for anything different.

dihm commented 1 year ago

I did spend a little time looking in to the deprecation warnings. It appears that setuptools highly discourages using multiple top-level packages in a flat layout and instead recommends using a src folder to contain the packages. I played with that a bit and it works pretty well along with the find_namespace functionality. It does change the editable install method to a plain pth file, for whatever that's worth. We may want to consider implementing this along with the actual required fix.

chrisjbillington commented 1 year ago

Nice to know about the deprecation warnings and the src layout alternative. Presumably if we explicitly list packages in setup.cfg it will stop complaining as well?

I don't mind moving things to a src layout, though the functionality I just added to desktop_app to find editable install paths will need to be extended to include this case as well (not hard).

But wouldn't be high on any priority list I would think.