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

Project-based venv #90

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Project-based venv #90

wants to merge 8 commits into from

Conversation

asmodehn
Copy link
Contributor

@asmodehn asmodehn commented Jan 3, 2023

This depends on #89

Adds a target to the makefile, to deploy to a project based venv.
This allows to gather dependencies into a requirements.txt file.

Also installing in a clean virtual env from the wheel revealed missing dependencies in setup.py

Possible future improvements:

  • separate system management commands, and python project development commands, currently both conflated in same makefile (but with potentially very different impact on the user's machine).
  • have a separate version file, so we don't need to import the whole opengeode package (with potentially missing dependencies) when grabbing the version.


# optional dependencies
stringtemplate3
singledispatch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why optional? singledispatch is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code tells me the singledispatch decorator is required, but the singledispatch package is needed only for python2 (backport).
With python3, singledispatch is imported from the package functools.

Also python documentation says that the singledispatch decorator is part of functools since version 3.4.

Should we care about supporting python versions <=3.4 ?
TASTE is using 3.5 from what I can recall, and the setup.py says >=3.7 ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh so the singledispatch package is unnecessary now and indeed this dependency could be removed (and replaced with an import from functools). I missed that, thanks for spotting it.

@asmodehn
Copy link
Contributor Author

asmodehn commented Jan 25, 2023

I recently rebased this onto the recent makefile_improvements.
Things are working fine.
I can make develop to work from a project-based venv.
I can also build a wheel and install it via pip:

$ pip install dist/opengeode-4.0.2-py3-none-any.whl 
Processing ./dist/opengeode-4.0.2-py3-none-any.whl
Requirement already satisfied: pygraphviz in /home/alexv/.local/lib/python3.10/site-packages (from opengeode==4.0.2) (1.10)
Requirement already satisfied: pyside6 in /home/alexv/.local/lib/python3.10/site-packages (from opengeode==4.0.2) (6.4.2)
Requirement already satisfied: shiboken6==6.4.2 in /home/alexv/.local/lib/python3.10/site-packages (from pyside6->opengeode==4.0.2) (6.4.2)
Requirement already satisfied: PySide6-Addons==6.4.2 in /home/alexv/.local/lib/python3.10/site-packages (from pyside6->opengeode==4.0.2) (6.4.2)
Requirement already satisfied: PySide6-Essentials==6.4.2 in /home/alexv/.local/lib/python3.10/site-packages (from pyside6->opengeode==4.0.2) (6.4.2)
Installing collected packages: opengeode
  Attempting uninstall: opengeode
    Found existing installation: opengeode 4.0.2
    Uninstalling opengeode-4.0.2:
      Successfully uninstalled opengeode-4.0.2
Successfully installed opengeode-4.0.2

One thing I don't really like is the fact that I have to activate the virtual env from the makefile at every line there (make will create a new shell process for each line, to mitigate side-effects damage, and therefore we loose the virtualenv).

Which is why I would like to move the "non-obvious" steps outside the makefile if possible (in a later PR):

  • dealing with antlr3_python3_runtime as a proper pip dependency, somehow...
  • doing pyside6-rcc opengeode.qrc -o opengeode/icons.py manually, or automate it in a different way, and put icons.py in repo ?...
    Let me know if you have any idea about those, or if you prefer the PR as is now.

@asmodehn asmodehn marked this pull request as ready for review January 25, 2023 10:41
@maxime-esa
Copy link
Contributor

One thing I don't really like is the fact that I have to activate the virtual env from the makefile at every line there (make will create a new shell process for each line, to mitigate side-effects damage, and therefore we loose the virtualenv).

Why don't you use && between each command to avoid this ?

Also I am not sure to understand - the install rule depends on develop. Does it mean that it will be installed twice - first in the venv then on the system ?

Which is why I would like to move the "non-obvious" steps outside the makefile if possible (in a later PR):
* dealing with antlr3_python3_runtime as a proper pip dependency, somehow...

Yes the solution would be to upload this runtime on PyPi, making it available properly to everyone

* doing `pyside6-rcc opengeode.qrc -o opengeode/icons.py` manually, or automate it in a different way, and put icons.py in repo ?...

In general I don't want to put generated code on the repo

@asmodehn
Copy link
Contributor Author

One thing I don't really like is the fact that I have to activate the virtual env from the makefile at every line there (make will create a new shell process for each line, to mitigate side-effects damage, and therefore we loose the virtualenv).

Why don't you use && between each command to avoid this ?

Sure, I could do this, but it is perhaps less clear which command will embed which shell (and how errors will be back-propagated), and anyway I see it as a hint that doing so many steps in the Makefile is not the way to go about it...

Also I am not sure to understand - the install rule depends on develop. Does it mean that it will be installed twice - first in the venv then on the system ?

Correct.
Note I only install it on the venv to be able to generate the resources with pyside6-rcc, so that the file is present when installing afterwards...

Which is why I would like to move the "non-obvious" steps outside the makefile if possible (in a later PR):

  • dealing with antlr3_python3_runtime as a proper pip dependency, somehow...

Yes the solution would be to upload this runtime on PyPi, making it available properly to everyone

For later reference, there seems to be a way to bring it along from opengeode setup.py directly, called direct references : https://peps.python.org/pep-0440/#direct-references
This would allow to install it when installing opengeode, without having to make it a pip package by itself.
Disclaimer, I havent tried that feature yet.

* doing `pyside6-rcc opengeode.qrc -o opengeode/icons.py` manually, or automate it in a different way, and put icons.py in repo ?...

In general I don't want to put generated code on the repo

Here is an interesting option: do it in the setup.py : https://gist.github.com/ivanalejandro0/6758741
This way we could hook it to existing commands. Ref: https://setuptools.pypa.io/en/latest/userguide/extension.html#customizing-commands or maybe even via a build sub-command since we just deterministically build a file here: https://setuptools.pypa.io/en/latest/userguide/extension.html#supporting-sdists-and-editable-installs-in-build-sub-commands

@maxime-esa
Copy link
Contributor

  • dealing with antlr3_python3_runtime as a proper pip dependency, somehow...

Yes the solution would be to upload this runtime on PyPi, making it available properly to everyone

For later reference, there seems to be a way to bring it along from opengeode setup.py directly, called direct references : https://peps.python.org/pep-0440/#direct-references This would allow to install it when installing opengeode, without having to make it a pip package by itself. Disclaimer, I havent tried that feature yet.

What about python3 -m pip install --user https://download.tuxfamily.org/taste/antlr3_python3_runtime_3.4.tar.bz2 ?
That seems to work as well.

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.

2 participants