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

Modernise and rationalise packaging #1161

Closed
joshuagl opened this issue Oct 5, 2020 · 11 comments · Fixed by #1626
Closed

Modernise and rationalise packaging #1161

joshuagl opened this issue Oct 5, 2020 · 11 comments · Fixed by #1626
Assignees

Comments

@joshuagl
Copy link
Member

joshuagl commented Oct 5, 2020

Description of issue or feature request:

Python packaging has changed significantly since the tuf packaging was created. There's been a move to declarative files, the formalisation of build system independent source tree layout in PEP 517, and the addition of new metadata to specify the build system dependencies in PEP 518.

Furthermore our current packaging is somewhat ad-hoc, with some expected files like LICENSE missing from the sdist (see #1160) and some unexpected files like tuf/ATTACKS.md included.

For reference see:
https://packaging.python.org/
https://snarky.ca/what-the-heck-is-pyproject-toml/

Current behavior:

Packaging isn't up-to-date with PyPA recommendations and our sdist includes files which don't match conventions.

Expected behavior:

Packaging is in line with current PyPA best practices and generated distributions match ecosystem conventions.

@joshuagl
Copy link
Member Author

joshuagl commented Oct 5, 2020

We should add some CI steps to ensure our packaging metadata is sane, for example our current MANIFEST.in fails a check-manifest test with:

lists of files in version control and sdist do not match!

@jku
Copy link
Member

jku commented Jan 20, 2021

A gotcha that we may care about with regards to setup.py/setup.cfg/pyproject.toml: I just tried to do pip install -e <directory> to a project that only has a pyproject.toml and:

pip install -e ../vendoring
ERROR: File "setup.py" not found. Directory cannot be installed in editable mode: /home/jku/src/vendoring
(A "pyproject.toml" file was found, but editable mode currently requires a setup.py based build.)

Which would mean current requirements-dev.txt in tuf would fail to install in this case.

@sechkova
Copy link
Contributor

A gotcha that we may care about with regards to setup.py/setup.cfg/pyproject.toml: I just tried to do pip install -e <directory> to a project that only has a pyproject.toml and:

pip install -e ../vendoring
ERROR: File "setup.py" not found. Directory cannot be installed in editable mode: /home/jku/src/vendoring
(A "pyproject.toml" file was found, but editable mode currently requires a setup.py based build.)

Which would mean current requirements-dev.txt in tuf would fail to install in this case.

Yep, it is a known limitation, it can be circumvented by:

import setuptools

if __name__ == "__main__":
    setuptools.setup()

but needs to be kept in mind (link).

joshuagl added a commit to joshuagl/tuf that referenced this issue Mar 2, 2021
Remove references to, and handling of, Python 2.7 in our project scaffolding:
- updated python_requires in setup.py to state our intent to support
  Python 3.6 and above (but not Python 4, yet)
- Drop no longer required dependencies in setup.py, and requirements-*.txt
  (further refinement of requirements files will be handled in theupdateframework#1161)
- Remove Python 2.7 from our tox environments

Signed-off-by: Joshua Lock <[email protected]>
@sechkova
Copy link
Contributor

sechkova commented Mar 2, 2021

I am sharing, after of a brief research on the topic, a list of the current issues with suggestions on how to move forward.
Please (dis)agree, add missing information, correct me if I am wrong.

1. Lagging behind from the latest PyPA recommendations and PEPs (517, 518)

The obvious proposal here is to just follow the PyPA guideline which recommends:

  • adopting a pyproject.toml (PEP 518) which describes the project's build dependencies
  • keeping setuptools as the packaging tool given this is already in use and nothing strongly enforces its replacement
  • switching to a static setuptools config, setup.cfg
  • using build

* keep in mind the two comments above related to an issue when installing in editable mode.

This should make the project PEP 517, PEP 518 compliant which also gives the opportunity to easily migrate to another packaging tool if such need arrives in the future. Just listing some popular names here: flit, poetry, bento

2. File gathering: MANIFEST.in is too fine grained, misses needed files while adding irrelevant ones

  • Use setuptools_scm extension and list only files that need to be excluded in MANIDEST.in
  • If still relevant, add tools like check-manifest to the GitHub Actions

3. Semi-manual requirements*.txt generation for different Python versions

  • Hopefully this will be resolved, at least the manual part should be greatly reduced, by dropping Python 2.7. support
  • Nothing seems to be changed on the topic of "why pin dependencies" so I won't raise it here but I am adding some links for a historical background: 730e4d8, Revise dependency handling in-toto/in-toto#294

Adding some blogposts to Joshua's list of references:
https://snarky.ca/clarifying-pep-518/
https://snarky.ca/what-the-heck-is-pyproject-toml/
https://blog.ionelmc.ro/presentations/packaging/
https://caremad.io/posts/2013/07/setup-vs-requirement/

joshuagl added a commit to joshuagl/tuf that referenced this issue Mar 2, 2021
Remove references to, and handling of, Python 2.7 in our project scaffolding:
- updated python_requires in setup.py to state our intent to support
  Python 3.6 and above (but not Python 4, yet)
- Drop no longer required dependencies in setup.py, and requirements-*.txt
  (further refinement of requirements files will be handled in theupdateframework#1161)
- Remove Python 2.7 from our tox environments

Signed-off-by: Joshua Lock <[email protected]>
joshuagl added a commit to joshuagl/tuf that referenced this issue Mar 3, 2021
Remove references to, and handling of, Python 2.7 in our project scaffolding:
- updated python_requires in setup.py to state our intent to support
  Python 3.6 and above (but not Python 4, yet)
- Drop no longer required dependencies in setup.py, and requirements-*.txt
  (further refinement of requirements files will be handled in theupdateframework#1161)
- Remove Python 2.7 from our tox environments

Signed-off-by: Joshua Lock <[email protected]>
@joshuagl
Copy link
Member Author

joshuagl commented Mar 3, 2021

Thank you for doing the research Teodora!

  1. seems very reasonable. build gives us build isolation, which in my opinion warrants the switch. What else do we gain by switching to build? Is it just a simpler/more consistent interface for building than invoking setup.py?
  2. seems reasonable also
  3. Python 2.7 is gone 🎉

It would be great to handle the things you listed in 1. and the setuptools_scm change suggested in 2 to address this issue.
The items in 3 can reasonably handled separately as a fix for #1249.

@lukpueh
Copy link
Member

lukpueh commented Mar 9, 2021

Thanks for the great summary, @sechkova! I second @joshuagl's comment, plus:

  1. ... gives the opportunity to easily migrate to another packaging tool if such need arrives in the future. Just listing some popular names ...

Any recommendations on tools?

  1. ... Nothing seems to be changed on the topic of "why pin dependencies"...

Did you see how other projects do this? in-toto/in-toto#294 -- i.e. un-pinned direct deps for user installs vs. pinned + auto-updated direct and transitive deps for CI/CD -- felt like a good but very custom solution. I'd be curious if there are better or more common practices. Also in regards to pinning direct and transitive deps for different Python versions. Although the latter might really just not be relevant without Python 2.7. IIRC TUF has no dependency case-handling for different Python 3 versions.

@sechkova
Copy link
Contributor

sechkova commented Mar 9, 2021

Any recommendations on tools?

I am wary of recommending tools that I haven't used, especially with no experience in the packaging world. However, @joshuagl may be interested in flit which has some support for reproducible builds .
I will share if I come across something promising.

On the second part of the problem, dependencies resolution, I couldn't find any different practices (than what we do) besides suggestions to totally switch to Poetry (+ Pipenv). I suggest we revise such necessity after removing py2-related dependencies.

@sechkova
Copy link
Contributor

sechkova commented Mar 9, 2021

  1. seems very reasonable. build gives us build isolation, which in my opinion warrants the switch. What else do we gain by switching to build? Is it just a simpler/more consistent interface for building than invoking setup.py?

My understanding at this point is that build simply "invokes the correct PEP 517 hooks to build a distribution package". Maybe the end result will be the same as with setup.py since we use setuptools anyway or maybe some config may misbehave ... I can only speculate before trying it.

@joshuagl
Copy link
Member Author

joshuagl commented Mar 9, 2021

Any recommendations on tools?

I am wary of recommending tools that I haven't used, especially with no experience in the packaging world. However, @joshuagl may be interested in flit which has some support for reproducible builds .

Ah, interesting. flit might be a good stop-gap for achieving #1269. I'll mention it in that issue, thanks @sechkova !

lukpueh added a commit to lukpueh/tuf that referenced this issue Mar 12, 2021
Configure lint build in tox.ini to check if code in tuf/api/* is
formatted according to black and isort style rules:
https://black.readthedocs.io/en/stable/the_black_code_style.html
https://pycqa.github.io/isort/

In addition to our new style guide (theupdateframework#1128) and corresponding linter
configuration, requiring auto-formatting should help to further
reduce reviewing effort. The auto-formatter black was chosen for
the following reasons:
- It seems to be the most popular formatter in the Python ecosystem
- It is well documented including integration instructions with
  most of the tools we use (git, GitHub Actions, pylint, a range of
  editors, pyproject.toml theupdateframework#1161)
- It checks that the reformatted code produces a valid AST that is
  equivalent to the original
- It has almost no ways of customization, which means no
  customization effort required, and more (cross-project) style
  uniformity, lowering contribution barriers
- It converts single to double quotes, where reasonable, which is
  exactly what we recommend
- The style choices it makes seem generally reasonable and don't
 conflict with our style guide, except for favoring hanging over
 aligned indentation, which is the opposite of what we recommend.
 But we are willing to update the adapt our style guide.

Auto-format pre-commit configuration will be added in a subsequent
commit.

Signed-off-by: Lukas Puehringer <[email protected]>
rzr added a commit to CrossStream/tuf that referenced this issue May 8, 2021
Those generated files are not explicitly under Apache-2.0 licence
and AFAIK they can not be regenerated from missing (latex?) sources.

To avoid licence mixup.
It would help to have those files published elsewhere.
Meanwhile online (Github) links are used.

Debian had to repack the source package to make tarball compliant with DFSG
dispite debian tools are known to be trustworthy,
this extra step would add weakess in the chain of trust

Cleanup done upstream would make distribution safer.

Bug: theupdateframework#1161
Bug-Debian: https://salsa.debian.org/python-team/packages/tuf/-/merge_requests/11
Relate-to: theupdateframework#263 (comment)
Signed-off-by: Philippe Coval <[email protected]>
@rzr rzr mentioned this issue May 8, 2021
3 tasks
rzr added a commit to CrossStream/tuf that referenced this issue May 8, 2021
Those generated files are not explicitly under Apache-2.0 licence
and AFAIK they can not be regenerated from missing (latex?) sources.

To avoid licence mixup.
It would help to have those files published elsewhere.
Meanwhile online (Github) links are used.

Debian had to repack the source package to make tarball compliant with DFSG
despite debian tools are known to be trustworthy,
this extra step would add weakess in the chain of trust

Cleanup done upstream would make distribution safer.

Bug: theupdateframework#1161
Bug-Debian: https://salsa.debian.org/python-team/packages/tuf/-/merge_requests/11
Relate-to: theupdateframework#263 (comment)
Forwarded: theupdateframework#1380
Signed-off-by: Philippe Coval <[email protected]>
rzr added a commit to CrossStream/tuf that referenced this issue May 10, 2021
Duplication is not needed since files are hosted in website project:

https://github.com/theupdateframework/theupdateframework.io/tree/master/static/papers

Those generated files are not explicitly under Apache-2.0 licence
and AFAIK they can not be regenerated from missing (latex?) sources.

To avoid licence mixup.
It would help to have those files published elsewhere.
Meanwhile online (Github) links are used.

Debian had to repack the source package to make tarball compliant with DFSG
despite debian tools are known to be trustworthy,
this extra step would add weakess in the chain of trust

Cleanup done upstream would make distribution safer.

Bug: theupdateframework#1161
Bug-Debian: https://salsa.debian.org/python-team/packages/tuf/-/merge_requests/11
Relate-to: theupdateframework#263 (comment)
Forwarded: theupdateframework#1380
Relate-to: theupdateframework/specification#160
Signed-off-by: Philippe Coval <[email protected]>
trishankatdatadog pushed a commit that referenced this issue May 28, 2021
Duplication is not needed since files are hosted in website project:

https://github.com/theupdateframework/theupdateframework.io/tree/master/static/papers

Those generated files are not explicitly under Apache-2.0 licence
and AFAIK they can not be regenerated from missing (latex?) sources.

To avoid licence mixup.
It would help to have those files published elsewhere.
Meanwhile online (Github) links are used.

Debian had to repack the source package to make tarball compliant with DFSG
despite debian tools are known to be trustworthy,
this extra step would add weakess in the chain of trust

Cleanup done upstream would make distribution safer.

Bug: #1161
Bug-Debian: https://salsa.debian.org/python-team/packages/tuf/-/merge_requests/11
Relate-to: #263 (comment)
Forwarded: #1380
Relate-to: theupdateframework/specification#160
Signed-off-by: Philippe Coval <[email protected]>
@joshuagl
Copy link
Member Author

joshuagl commented Sep 6, 2021

PyPA are recommending build now, we should switch to that (ensuring that docs/RELEASE.md is updated too).

@joshuagl
Copy link
Member Author

Not only are PyPA recommending build, but direct invocations of setup.py are considered deprecated for a number of years!? 😱

https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html

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 a pull request may close this issue.

4 participants