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: parse requirements.txt with locally referenced packages #315

Closed
madpah opened this issue Dec 15, 2021 · 20 comments · Fixed by #327
Closed

feat: parse requirements.txt with locally referenced packages #315

madpah opened this issue Dec 15, 2021 · 20 comments · Fixed by #327
Assignees
Labels
enhancement New feature or request

Comments

@madpah
Copy link
Collaborator

madpah commented Dec 15, 2021

As per issue #284 raised by @Jonas-vdb.

Some development teams add local references to other packages in their requirements.txt files. This is currently causing an exception:

Traceback (most recent call last):
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/packaging/requirements.py", line 98, in __init__
    req = REQUIREMENT.parseString(requirement_string)
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/pyparsing.py", line 1654, in parseString
    raise exc
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/pyparsing.py", line 1644, in parseString
    loc, tokens = self._parse( instring, 0 )
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/pyparsing.py", line 1402, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/pyparsing.py", line 3417, in parseImpl
    loc, exprtokens = e._parse( instring, loc, doActions )
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/pyparsing.py", line 1402, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/pyparsing.py", line 3739, in parseImpl
    return self.expr._parse( instring, loc, doActions, callPreParse=False )
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/pyparsing.py", line 1402, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/pyparsing.py", line 3400, in parseImpl
    loc, resultlist = self.exprs[0]._parse( instring, loc, doActions, callPreParse=False )
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/pyparsing.py", line 1406, in _parseNoCache
    loc,tokens = self.parseImpl( instring, preloc, doActions )
  File "/home/jvdb/repos/venv-tmp/lib/python3.6/site-packages/pkg_resources/_vendor/pyparsing.py", line 2711, in parseImpl
    raise ParseException(instring, loc, self.errmsg, self)
pkg_resources._vendor.pyparsing.ParseException: Expected W:(abcd...) (at char 0), (line:1, col:1)
@madpah madpah added bug Something isn't working enhancement New feature or request labels Dec 15, 2021
@madpah madpah self-assigned this Dec 15, 2021
@madpah
Copy link
Collaborator Author

madpah commented Dec 15, 2021

Root Cause: pkg_resources again does not support parsing requirements of this format (similar to requirements with hashes included) - see here.

Also - worth stating that given the example provided, we will not be able to determine the version for any locally referenced requirements through a requirements.txt method. Other methods such as EnvironmentParser would pick them up.

madpah referenced this issue in CycloneDX/cyclonedx-python-lib Dec 15, 2021
Signed-off-by: Paul Horton <phorton@sonatype.com>
@jkowalleck
Copy link
Member

@madpah i dont see an actual urge for this feature, and i dont see it as a bug.
relative paths are not to be expected to be output of pip freeze

but it is a convenient feature, no doubt.

@madpah
Copy link
Collaborator Author

madpah commented Dec 21, 2021

I agree @jkowalleck - will remove the bug label.

@madpah madpah removed the bug Something isn't working label Dec 21, 2021
@mostafa
Copy link
Contributor

mostafa commented Feb 17, 2022

@madpah
I was thinking about making a PR to replace pkg_resources with requirements-parser because the latter can parse all sorts of requirements. WDYT?

@madpah
Copy link
Collaborator Author

madpah commented Feb 18, 2022

Hey @mostafa - thanks for getting involved.

This was absolutely my plan. However, since this ticket was created, the logic/classes that use pkg_resources now live in an upstream project cyclonedx-python - so I'll first relocate this issue (back) there.

Would be great to get a PR from you to make this change, but worth nothing we have a large(ish) release due to land probably next week now, so you might want to wait until that is merged before you branch.

@madpah madpah transferred this issue from CycloneDX/cyclonedx-python-lib Feb 18, 2022
@madpah
Copy link
Collaborator Author

madpah commented Feb 18, 2022

This issue was moved back to cyclonedx-python as since version 2.0.0 of cyclonedx-python-lib, Python specific parsers now reside in cyclonedx-python project.

@mostafa
Copy link
Contributor

mostafa commented Feb 18, 2022

Perfect! Just give me a nod when you release next week. Alternatively, I can start working on the feature off the branch you want to merge and then rebase from the default branch after merging. Up to you! 🙂

@madpah
Copy link
Collaborator Author

madpah commented Feb 21, 2022

Hey @mostafa - we've released cyclonedx-python 3.0.0 today. Would be awesome to get a PR to move to requirements-parser as you suggested.

Thanks!

@mostafa
Copy link
Contributor

mostafa commented Feb 22, 2022

@madpah I forked the master branch and tried to test the library locally but ran into an issue:
The tests fail because the method signature of BaseParser.get_components returns self._components which is a List[Component], but the code expects a set, so it can add to it; a method that is not supported on a list. There are 4 instances of this issue in various places. The following is an example of the tests that fail because of this error:

======================================================================
ERROR: test_conda_list_explicit_md5 (tests.test_parser_conda.TestCondaParser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/somewhere/cyclonedx-python/tests/test_parser_conda.py", line 46, in test_conda_list_explicit_md5
    parser = CondaListExplicitParser(conda_data=conda_list_ouptut_fh.read())
  File "/somewhere/cyclonedx-python/cyclonedx_py/parser/conda.py", line 41, in __init__
    self._conda_packages_to_components()
  File "/somewhere/cyclonedx-python/cyclonedx_py/parser/conda.py", line 68, in _conda_packages_to_components
    c.external_references.add(ExternalReference(
AttributeError: 'list' object has no attribute 'add'

Is this something you're aware of? Or should I create a separate issue?

@madpah
Copy link
Collaborator Author

madpah commented Feb 22, 2022

@mostafa - not aware of this issue, and concerned as tests are passing in CI too. If you can raise an Issue for this, I’ll investigate later today.

Thanks again 🙏

@madpah
Copy link
Collaborator Author

madpah commented Feb 22, 2022

Tests passing here: https://github.com/CycloneDX/cyclonedx-python/runs/5277735507?check_suite_focus=true

@mostafa
Copy link
Contributor

mostafa commented Feb 22, 2022

@madpah I saw the passing tests, which is why I am confused, but I'll create a separate issue for it.

@mostafa
Copy link
Contributor

mostafa commented Feb 22, 2022

@madpah It's a dependency issue on my machine. So, my bad. 🤦

@llamahunter
Copy link
Contributor

We don't have locally referenced packages, but we do have a private pypi repo, and it appears that the pkg_resources library simply isn't up to the task of parsing a full pip requirements.txt file, including the --index-url directives.
#318

@jkowalleck
Copy link
Member

jkowalleck commented Feb 23, 2022

Tool might not supports all the features people use in their requirements.txt.
the capability is a file that is created by pip freeze. @llamahunter

please open another issue, if you want to request another feature for the requirements parser.
this issue is about local packages.

@jkowalleck jkowalleck changed the title Support for locally referenced packages in requirements.txt feat: requirements.txt with locally referenced packages Feb 23, 2022
@jkowalleck jkowalleck changed the title feat: requirements.txt with locally referenced packages feat: parse requirements.txt with locally referenced packages Feb 23, 2022
@mostafa
Copy link
Contributor

mostafa commented Feb 23, 2022

@madpah @jkowalleck
I had some issues with a test (test_example_multiline_with_comments) not passing after using the requirements-parser project because it doesn't support either the hashes or multiline requirements (either or both) and I found that there's a more mature alternative, the pip-requirements-parser. WDYT? As you're the maintainer/developer, do you strictly want to use and develop the requirements-parser package?

Also, the pip-requirements-parser package has only one API, which is RequirementsFile.from_file, and if we want to parse requirements file as a string, we need to make our own API using internal APIs of the package or get rid of our string processing altogether.

@jkowalleck
Copy link
Member

jkowalleck commented Feb 23, 2022

just for the record: I do not have any opinion about a particular requirements-file-parser. Unlike @madpah I never dug deep into this topic.


regarding

Also, the pip-requirements-parser package has only one API, which is RequirementsFile.from_file, and if we want to parse requirements file as a string, we need to make our own API using internal APIs of the package or get rid of our string processing altogether.

For string input we could create a temp file and run RequirementsFile.parse/RequirementsFile.from_file on the temp file with include_nested=False, as all relative paths would be bare assumptions out of thin air based on current working dir and stuff.
For direct file input we would set include_nested=true, as relative paths from that file could be resolved.

@llamahunter
Copy link
Contributor

please open another issue, if you want to request another feature for the requirements parser.
this issue is about local packages.

I already did. See my post above and #318

@jkowalleck
Copy link
Member

for discussions, feel free to switch to #319

@mostafa
Copy link
Contributor

mostafa commented Feb 23, 2022

One more thing that's definitely off-topic:
I see lots of <file>.close() lines throughout the project inside context managers, which is redundant because the file itself is actually being opened inside the body of a with statement that is a context manager and will close the file automatically, even if an exception occurs inside the body.

@madpah madpah linked a pull request Mar 10, 2022 that will close this issue
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
Development

Successfully merging a pull request may close this issue.

4 participants