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

Revise requirements files and remove pyup #978

Merged
merged 5 commits into from
Feb 11, 2020

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Feb 6, 2020

Fixes issue #:
None

Description of the changes being introduced by the pull request:

  • Use suffixed instead of prefixed sub-requirements files to group them alphabetically in the file tree.

  • Layer requirements files akin to the in-toto project (see Revise dependency handling in-toto/in-toto#294). The hierarchy is:

    • requirements.in
      tuf runtime requirements, including optional requirements (pynacl and cyrptography)

    • requirements-pinned.txt
      pinned tuf runtime requirements, including optional and transitive (1 level deep) requirements and their hashes.

      The file is generated semi-automatically using pip-compile and a bash script (see document header), based off of requirements.in, combining requirements from all supported Python versions.

      This file should be auto-updated, by e.g. dependabot, and be used for ci/cd tests, to catch issues with new dependencies.

    • requirements-test.txt
      additional test runtime requirements

    • requirements-tox.txt
      combines requirements.txt, requirements-test.txt and additional test tools (for linting and coverage), i.e. everything that is needed in each tox environment to run the tests.

    • requirements-dev.txt
      lists tox for local development and testing, and also requirements-tox.txt and tuf in editable mode to run the test suite or individual tests directly.

    • requirements.txt
      requirements-pinned.txt with the hashes of the dependencies as reported by pip at the time of creating the file. NOTE: this is not used for testing or dev-install because pip doesn't allow mixed (with and without hashes) installations.

      This file should also be auto-updated, by e.g. dependabot.

  • Remove an obsolete version constraint on coverage

  • Remove pyup config and badges (replace with dependabot)

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@trishankatdatadog
Copy link
Member

@lukpueh Is this PR done or WIP? :)

* Use suffixed instead of prefixed sub-requirements files to group
  them alphabetically in the file tree.
* Layer requirements files akin to the in-toto project
  (see in-toto/in-toto#294). The hierarchy is:

  - *requirements.in*
    tuf runtime requirements, including optional requirements
    (pynacl and cyrptography)

  - *requirements-pinned.txt*
    pinned tuf runtime requirements, including optional
    and transitive (1 level deep) requirements and their hashes.

    The file is generated semi-automatically using pip-compile
    and a bash script (see document header), based off of
    requirements.in, combining requirements from all supported
    Python versions.

    This file should be auto-updated, by e.g. dependabot, and be used
    for ci/cd tests, to catch issues with new dependencies.

  - *requirements-test.txt*
    additional test runtime requirements

  - *requirements-tox.txt*
    combines requirements.txt, requirements-test.txt and additional
    test tools (for linting and coverage), i.e. everything that is
    needed in each tox environment to run the tests.

  - *requirements-dev.txt*
    lists tox for local development and testing, and also
    requirements-tox.txt and tuf in editable mode to run
    the test suite or individual tests directly.

  - *requirements.txt*
    requirements-pinned.txt with the hashes of the dependencies
    as reported by pip at the time of creating the file.
    NOTE: this is not used for testing or  dev-install because pip
    doesn't allow mixed (with and without hashes) installations.

    This file should also be auto-updated, by e.g. dependabot.

* Removes an obsolete version constraint on coverage

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
The PyUp GitHub integration for TUF stopped working a few days
ago. Instead of troubleshooting, I'm seizing the opportunity to
replace it with Dependabot, which has shown to work well in the
in-toto org.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh
Copy link
Member Author

lukpueh commented Feb 6, 2020

1a826cb fixes an unrelated test issue, due to a recent sslib update

@lukpueh
Copy link
Member Author

lukpueh commented Feb 6, 2020

Thanks for the interest, @trishankatdatadog. I just fixed an unrelated minor issue that made the tests fail. If travis returns all good, this should be ready for review.

@trishankatdatadog
Copy link
Member

Thanks for your hard work, @lukpueh!

So many requirements files 😅 Are they really necessary? Also, is there an automated to update all of them at once?

@joshuagl
Copy link
Member

joshuagl commented Feb 6, 2020

This looks good to me, dependabot will monitor dependencies and update them and yet per in-toto/in-toto#294 this is versatile enough for downstreams to package and developers to install where the pinned versions aren't available.

One thing missing from the changes which is listed in the PR is addition of dependabot badges 😄

Note: the AppVeyor failure is the regular Windows+Python2.7 failure in #965.

@lukpueh
Copy link
Member Author

lukpueh commented Feb 7, 2020

Thanks for your hard work, @lukpueh!

👍

So many requirements files 😅 Are they really necessary?

Hehe. Judge for yourself. The PR description provides details about each file.

I guess I could merge requirements-test.txt and requirements-tox.txt into one file. Semantically, the separation makes sense because the former corresponds to tests_require in setup.py, and the latter installs that and everything else that's need for a tox build.

I also think we could do with just one of requirements.txt (pinned with hashes) and requirements-pinned.txt (pinned without hashes). The reason I need one without hashes is that pip doesn't accept requirements files that mix requirements with and without hashes (or editable installs), which is hard to work around (see e.g. requirements-tox.txt and tox.ini#L23-L26). The one with hashes I kept because users might want the integrity guarantees it means to provide. I think they are not very useful though, especially with PEP 458 coming.

Regardless, if it's still too many files, I can hide them in a subdirectory. ;)

Also, is there an automated to update all of them at once?

Yes. The only files that need to be updated regularly are the ones with pinned requirements, i.e. requirements.txt and requirements-pinned.txt. This happens automatically through dependabot, which triggers automatic builds to check that tuf tests indeed pass with the newest dependencies.

Additionally, there's a semi-automated process (a copy-pastable bash multi-liner as comment in the file), which should be re-executed if requirements are added to (or removed from) requirements.in.

Makes sense? I'll add some comments to the files to clarify their purpose.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh
Copy link
Member Author

lukpueh commented Feb 7, 2020

Thanks for the review, @joshuagl. I added a status badge and also polished the comments a bit.

Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

I've suggested a couple of minor fixes (typo/misspelling and an additional shell comment so that a copy/paste of the script with the initial comments removed creates a runnable script).

I think it probably makes more sense (and makes the maintainers lives easier) to have the script as an executable file in the source tree, rather than a comment in the requirements.in?

requirements-tox.txt Outdated Show resolved Hide resolved
requirements.in Outdated Show resolved Hide resolved
Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Co-Authored-By: Joshua Lock <jlock@vmware.com>
@lukpueh
Copy link
Member Author

lukpueh commented Feb 7, 2020

Thanks, @joshuagl. I melded the fixes into my last commit. Regarding the script, you're probably right, I just didn't want to further pollute the repo with my dirty bash lines. (It felt more like a slightly longer command). :P

@lukpueh lukpueh merged commit 797d556 into theupdateframework:develop Feb 11, 2020
lukpueh added a commit to lukpueh/tuf that referenced this pull request Feb 18, 2020
Follows up on theupdateframework#978, which had the following problems:
- too many requirements files (cc @trishankatdatadog ;)
- used custom tooling around pip-compile, which prevented
  Dependabot from updating all files, because Dependabot "shells
  out" to pip-compile, making assumptions about the format of the
  compiled files, that we didn't meet.

This commit restructures the requirements files, choosing a much
simpler approach:

- Merges requirements-tox.txt and requirements-test.txt. The
  separation was semantically correct but operationally irrelevant.
- Removes the hashed requirements file, which doesn't add much
  security, especially with PEP 458 on the way (see python/peps#1306),
  but extra maintenance (see note about requirements.txt in theupdateframework#978).
- Removes the shell script that combined the results of pip-compile
  for all supported Python versions and instead pip-compiles for
  one Python version only. See comments about conditional transitive
  dependencies in requirements.txt in this PR for details.
@lukpueh lukpueh mentioned this pull request Feb 18, 2020
3 tasks
lukpueh added a commit to lukpueh/tuf that referenced this pull request Feb 18, 2020
Follows up on theupdateframework#978, which had the following problems:
- too many requirements files (cc @trishankatdatadog ;)
- used custom tooling around pip-compile, which prevented
  Dependabot from updating all files, because Dependabot "shells
  out" to pip-compile, making assumptions about the format of the
  compiled files, that we didn't meet.

This commit restructures the requirements files, choosing a much
simpler approach:

- Merges requirements-tox.txt and requirements-test.txt. The
  separation was semantically correct but operationally irrelevant.
- Removes the hashed requirements file, which doesn't add much
  security, especially with PEP 458 on the way (see python/peps#1306),
  but extra maintenance (see note about requirements.txt in theupdateframework#978).
- Removes the shell script that combined the results of pip-compile
  for all supported Python versions and instead pip-compiles for
  one Python version only. See comments about conditional transitive
  dependencies in requirements.txt in this PR for details.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh added a commit to lukpueh/tuf that referenced this pull request Feb 18, 2020
Follows up on theupdateframework#978, which had the following problems:
- too many requirements files (cc @trishankatdatadog ;)
- used extra tooling around pip-compile that
  - didn't take into account requirement markers (see comments
    in requirements.txt in this commit), and
  - confused Dependabot, which expects the hashed requirements
    file in a certain format, as pip-compile would generate it
    without custom tooling (see theupdateframework#979).

This commit restructures the requirements files as follows:

- Merges requirements-tox.txt and requirements-test.txt. The
  separation was semantically correct but operationally irrelevant.
- Removes the hashed requirements file, which doesn't add much
  security, especially with PEP 458 on the way (see python/peps#1306),
  but extra maintenance (see notes about requirements.txt in theupdateframework#978
  and about Dependabot above)
- Manually adds environment markers to requirements-pinned.txt (see
  comments in requirements.txt in this commit).

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh lukpueh mentioned this pull request Oct 22, 2020
3 tasks
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.

3 participants