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

v2dl3-vegas support for pypi install before and after PEP 517 and 621 #206

Open
wants to merge 3 commits into
base: pypi-eventdisplay
Choose a base branch
from

Conversation

tmcahill
Copy link
Contributor

Happy to lend a hand when I can. Github @ tag or E-mail the address on my profile.

  • fetch-depth: 0 ensures the entire git metadata is checked out for setuptools-scm

  • git config --global --add safe.directory should mostly only occur on VM's like Docker. I proposed adding a note about this to the README, since we documented and showed to users the docker recipes I made (V2DL3/utils/v2dl3-vegas-docker) as an easy way to run VEGAS and v2dl3-vegas without the heavy dependency setup (or on Windows, Mac, any Linux).

@GernotMaier I agree that it is preferable to remove setup.py. I attempted to on my test fork, and again when you raised the concern. On the CI, the install can run from the .toml file, but it does not install properly because the .toml is written using [package] syntax, supported by PEP 621+, which requires python 3.7+. The .toml may be able to be rewritten to PEP517, but that would essentially be setup.py wrapped in [tools.setuptools].

Unless VEGAS dependency support has been updated in the last year or so, it is difficult to get the latest Pythons working alongside a VEGAS install. VEGAS dependencies list Centos 6 or Ubuntu 16. The latest I could get working for the Docker recipes was Ubuntu 18, which supports up to Python 3.6. For these reasons, I would caution against a python 3.8 requirement. The install process will remain pip install . for all cases.

Setuptools doesn't seem to enforce its python requirement, perhaps for eventdisplay you might like to check python version at runtime and throw an error. Then only v2dl3-vegas would have to maintain the setup.py file, and we could add a comment stating such.

@tmcahill
Copy link
Contributor Author

@GernotMaier To answer your last question more specifically, fixes like touch setup.py to create a blank one or SCM_PRETEND_VERSION don't apply to this issue since the issue is not about getting setuptools to read the pyproject.toml, but rather that the pyproject.toml is written to a newer syntax that it cannot parse.

@GernotMaier
Copy link
Member

@tmcahill - I am trying to get this running, but pip install . is only working on python 3.6.

Did you mean above that you want to have python 3.6 as a requirement? I don't think this is something we want - this is an already quite old system and I think that V2DL3 should work in any modern python requirement. Would that be possible?

@tmcahill
Copy link
Contributor Author

tmcahill commented Feb 3, 2024

@tmcahill - I am trying to get this running, but pip install . is only working on python 3.6.

Did you mean above that you want to have python 3.6 as a requirement? I don't think this is something we want - this is an already quite old system and I think that V2DL3 should work in any modern python requirement. Would that be possible?

Interesting, I thought that the eventdisplay CI would test a python 3.8 install for me. Theoretically, it should have worked for all versions with that setup. I was able to reproduce your issue and tracked it down to setup.py's entry_points format being unreadable by newer build systems. I've changed it to a standard format and updated it to match your pyproject.toml scripts. It works when I test 3.6, 3.7, and 3.8.

A python 3.8 install will still parse setup.py, but the information in your .toml will be what is applied. You can confirm this by adding a dependency to the setup.py and doing a python 3.8 install; the package should not be installed. However if you add that dependency to the .toml, it will be installed. So setup.py should still function just as a setup file for legacy (python 3.6) builds.

I definitely agree that a more open python requirement is preferable. The .toml python requirement does correctly apply now. I have to remove it to get a python 3.7 install to work. A 3.6 install works regardless because it ignores the .toml. Perhaps you would consider changing the python requirement to >=3.7 ? If ED needs 3.8+, a separate warning or check could be applied.

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