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

Fixes .pth file has no effect in editable install #58

Closed philipstarkey closed 4 years ago

philipstarkey commented 4 years ago

Fixes #43

The .pth file is now copied during labscript-profile-create if it does not exist in the site-packages dir (where the .egg-info files exist) and if it the .pth file exists in the package structure (aka labscript-utils it is an editable install)

The labscript-suite.pth file has been updated to not raise an exception if labscript-suite has been uninstalled (so we do not have to worry about cleanup).

Also updated the labscript_profile.add_userlib_and_pythonlib function to use the site library for adding userlib and pythonlib to the system path.

philipstarkey commented 4 years ago

I have tested this for conda + developer install. I have not tested in the other install procedures (which will require using test PyPI/conda packages). It would be good to at least test with ediatble Windows Store Python (would appreciate someone else testing that as I don't have it installed)

philipstarkey commented 4 years ago

Gah, RTD fails due to a long standing bug using site within virtualenv (which RTD uses over venv for some reason).

This means that any project importing desktop-app will fail to build on RTD.

@chrisjbillington: I know this is ridiculous, but how would you feel about updating desktop-app to not call site.getsitepackages() unless it actually exists in site?

chrisjbillington commented 4 years ago

Sure, though I think there's a trick I saw somewhere about this problem, I'll see if I can find it otherwise just not call it.

chrisjbillington commented 4 years ago

Done, desktop-app now doesn't call those functions until they're needed.

Also, I made get_install_directory() public (in the sense of removing the prefixed underscore, anyway).

I'll have a go at adding a commit to your branch to use the new function name and depend on the new desktop-app version

rpanderson commented 4 years ago

Relevant:

chrisjbillington commented 4 years ago

Yes, I've got my eye on that Python issue. If they remove it they'll need to replace it with something else since plenty of projects use it defensible ways. We'll have to adapt to whatever that is when it comes. I've mentioned our use case in that thread, though that was back when we didn't care about the code execution and were just hard-coding paths.

chrisjbillington commented 4 years ago

One flaw with this approach (which didn't occur to me earlier) is that it only copies the .pth file if a profile is being created (duh).

This is a flaw because installation is not necessarily followed by profile creation - a profile might already exist. Perhaps we should add a custom develop command to our setup.py that runs the usual develop command and then copies the .pth file if so.

chrisjbillington commented 4 years ago

I took the liberty of moving the copying of the file into a custom develop command in setup.py (which is what gets run when you do pip install -e. This means the copying really is tied to creation of an editable install instead of profile creation.

Hopefully this is agreeable phil!

No desktop-app dependency is required this way since the setuptools machinery knows which site packages directory it is being installed to already.

The problem of the file being left behind remains, so the try: except:s to make it inert are still necessary.

philipstarkey commented 4 years ago

Sure. Should we be deleting the file as well if we're uninstalling? (I assume that's what self.uninstall` indicates?)

chrisjbillington commented 4 years ago

I did initially write code for that but pip doesn't actually call setup.py when uninstalling a package installed in editable mode, so the file will still be left behind

The uninstall code will run if a user runs python setup.py develop --uninstall, but they shouldn't really do that since they should stick to using pip.

Do you think I should put it back in? I lean against since it's a bit strange if it only works if the user is using non-recommended methods of managing their packages and we've otherwise accepted we're going to leave the file behind.

philipstarkey commented 4 years ago

Yeah, let's leave as is then. It's a bit dumb that pip doesn't call setup.py on uninstall...but then that seems to describe most aspect of python package management...

chrisjbillington commented 4 years ago

Lol. At least they acknowledged what a hodge-podge the ecosystem is by making their mascot a platypus