VERITAS-Observatory / V2DL3

VERITAS (VEGAS and Eventdisplay) to DL3 Converter
BSD 3-Clause "New" or "Revised" License
8 stars 1 forks source link

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

Open tmcahill opened 5 months ago

tmcahill commented 5 months ago

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

@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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

@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.