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

feat: Change requirements parser #327

Conversation

mostafa
Copy link
Contributor

@mostafa mostafa commented Mar 3, 2022

In this PR, I replaced the pkg_resources from setuptools with pip-requirements-parser. This change helps with better parsing of requirements.txt files because it supports local files, private repositories, hashes, and possibly other features. which I haven't looked into or tested. This PR tries to address the PRs mentioned in this discussion: #319.

As you might have noticed by now from the commits, these are the changes I made:

  1. Revamped the RequirementsParser class:
    • It now accepts path or file content.
    • Name of the package will be either the actual name or the URL, including file paths.
    • Version will be either the pinned version of the package or a list of versions (as string).
    • Hashes will be converted, processed, and then added to the Component.
  2. Added more tests:
    • test_example_local_packages tests references to local files with no nested file parsing.
    • test_example_local_and_nested_packages tests references to local files and remote URLs (pointing to files) that also include a parsed requirements line from a nested file.
    • test_example_private_packages: tests extra URLs.
    • test_example_with_urls: tests requirements as URLs.
  3. Unskipped the hashes test: test_example_with_hashes.
  4. Removed superfluous file.close() lines.
  5. Split lines where fixtures are opened to make them more readable.
  6. Minor update to the docs to mention that RequirementsFileParser now supports nested files.

@jkowalleck
Copy link
Member

@mostafa thanks in advance for all your work.

Please make sure to read and apply to the CONTRIBUTING guidelines ,
like signing your commits accordingly - which the DCO test is taking care of and currently complaining about. see the test details for fix instructions.

@jkowalleck jkowalleck changed the title (WIP) Feature: Change requirements parser [WIP] feat: Change requirements parser Mar 3, 2022
@jkowalleck jkowalleck added the enhancement New feature or request label Mar 3, 2022
@mostafa mostafa force-pushed the feat/parse-requirements-txt-with-locally-referenced-packages branch from b06339a to 1f2abb7 Compare March 4, 2022 12:52
Copy link
Contributor Author

@mostafa mostafa left a comment

Choose a reason for hiding this comment

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

Since I replaced the requirements parser, I think it fixed most of the issues and shortcomings of the previous one including nested file parsing, local and remote packages, etc. I followed @jkowalleck's suggestions by following and apply the CONTRIBUTING guidelines and at least the tests pass on my machine on Python 3.9 along with mypy and flake8.

I'd be very happy to have your feedback. @madpah @jkowalleck

cyclonedx_py/parser/poetry.py Show resolved Hide resolved
cyclonedx_py/parser/requirements.py Show resolved Hide resolved
cyclonedx_py/parser/requirements.py Outdated Show resolved Hide resolved
cyclonedx_py/parser/requirements.py Outdated Show resolved Hide resolved
cyclonedx_py/parser/requirements.py Show resolved Hide resolved
@mostafa mostafa marked this pull request as ready for review March 4, 2022 14:05
@mostafa mostafa requested a review from a team as a code owner March 4, 2022 14:05
@mostafa mostafa changed the title [WIP] feat: Change requirements parser feat: Change requirements parser Mar 4, 2022
@mostafa
Copy link
Contributor Author

mostafa commented Mar 4, 2022

Windows tests failed because of temp files nagging about permissions, so I tried to use a slightly different approach with roughly the same code: 333ca07.

Copy link
Collaborator

@madpah madpah left a comment

Choose a reason for hiding this comment

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

@mostafa - firstly - massive thanks for the PR. Great work.

Two questions from me raised in the review:

  1. To confirm the lowest version of the pip-requirements-parser that will work
  2. We need to update documentation to reflect your awesome work - let me know if you need a hand with this.

pyproject.toml Show resolved Hide resolved
tests/test_parser_requirements.py Show resolved Hide resolved
@madpah
Copy link
Collaborator

madpah commented Mar 8, 2022

@mostafa
Copy link
Contributor Author

mostafa commented Mar 8, 2022

@madpah Thanks for the review.

The PR hits multiple issues with the same stone. 😄

I'd be happy to write docs, too. So, I'll update the PR with docs changes soon.

cyclonedx_py/parser/requirements.py Outdated Show resolved Hide resolved
cyclonedx_py/parser/requirements.py Outdated Show resolved Hide resolved
@madpah
Copy link
Collaborator

madpah commented Mar 9, 2022

@mostafa - we merged #320 which has caused a conflict with poetry.lock - can you rebase and resolve?

mostafa added 9 commits March 9, 2022 16:09
…sion

Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
feat: Add support for hashes, local packages and private repositories
Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
…raryFileWrapper

Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
@mostafa mostafa force-pushed the feat/parse-requirements-txt-with-locally-referenced-packages branch from a78ac9e to 90b336f Compare March 9, 2022 15:09
mostafa added 2 commits March 9, 2022 17:06
Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
@mostafa
Copy link
Contributor Author

mostafa commented Mar 9, 2022

@madpah Can you give me a hand in updating the docs? 🙏

@mostafa mostafa requested review from jkowalleck and madpah March 9, 2022 17:20
@madpah
Copy link
Collaborator

madpah commented Mar 10, 2022

@madpah Can you give me a hand in updating the docs? 🙏

They are automatically published once we merge - your changes look to be good @mostafa !

@madpah
Copy link
Collaborator

madpah commented Mar 10, 2022

@mostafa - just looks like an issue reported by flake8 to resolve and we are good to merge.

Signed-off-by: Mostafa Moradian <mostafamoradian0@gmail.com>
@madpah madpah merged commit f973c91 into CycloneDX:master Mar 10, 2022
@mostafa mostafa deleted the feat/parse-requirements-txt-with-locally-referenced-packages branch March 10, 2022 15:00
@madpah
Copy link
Collaborator

madpah commented Mar 10, 2022

⭐ Massive thanks for this contribution @mostafa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants