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: fallback to gather metadata via pep517 if reading as Poetry project raises RuntimeError #5834

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

finswimmer
Copy link
Member

@finswimmer finswimmer commented Jun 11, 2022

At the moment Poetry gives up gather metadata from a Poetry project during installation/locking if parsing the pyproject.toml raises a RuntimeError. This currently happens if one tries to add a Poetry project whose pyproject.toml contains group-dependencies introduced in Poetry 1.2, but the Poetry in use is <1.2. Similar scenarios are possible in the future whenever something changed in the appearance of the pyproject.toml.

With this PR we catch the RuntimeError and fallback to pep517 to gather the metadata, making the process more robust.

Once accepted I would suggest to backport it to Poetry 1.1 and cut a new release immediately, hoping to make the transition period between 1.1 and 1.2 more smooth, by avoiding things like #5761.

  • Added tests for changed code.
  • Updated documentation for changed code.

@finswimmer finswimmer force-pushed the fallback-pep517 branch 2 times, most recently from 4cb31c9 to 0422772 Compare June 11, 2022 06:09
@finswimmer finswimmer requested a review from a team June 11, 2022 06:15
@finswimmer finswimmer force-pushed the fallback-pep517 branch 2 times, most recently from 3554f0b to 43d140e Compare August 21, 2022 06:20
tests/inspection/test_info.py Outdated Show resolved Hide resolved
tests/inspection/test_info.py Outdated Show resolved Hide resolved
@dimbleby
Copy link
Contributor

RuntimeError is a rather broad exception, and the scope at which it is suppressed is rather large in two of the three changed files. This risks masking errors other than the one you are aiming at.

  • too late to help if this does get back-applied to poetry 1.1: but can poetry-core be taught to throw a more specific exception on failing to parse the pyproject.toml?
  • can the scope of the exception handling be narrowed here?

@radoering
Copy link
Member

  • too late to help if this does get back-applied to poetry 1.1: but can poetry-core be taught to throw a more specific exception on failing to parse the pyproject.toml?

I agree we should do this at least for future versions.

  • can the scope of the exception handling be narrowed here?

Had the same thought and then forgot to mention it. It probably can since we are only expecting an exception from Factory.create_poetry(). At least that's my understanding.

@dimbleby
Copy link
Contributor

re throwing a more precise exception: there's an existing PyProjectException which would probably be fine both here and here, these should be the two that are relevant I think.

@finswimmer
Copy link
Member Author

  • can the scope of the exception handling be narrowed here?

Probably it can, but it looks a bit ugly:

try:
    package_poetry = Factory().create_poetry(pyproject.file.path.parent)
except RuntimeError:
    pass
else:
    builder: Builder
    if package.develop and not package_poetry.package.build_script:
        from poetry.masonry.builders.editable import EditableBuilder
        ...

Is it worth it? 🤔

@dimbleby
Copy link
Contributor

package_poetry = None
if pyproject.is_poetry_project():
    with contextlib.suppress(RuntimeError):
        package_poetry = Factory().create_poetry(pyproject.file.path.parent)

if package_poetry is not None:
    etc

?

@finswimmer
Copy link
Member Author

Much nicer 👍

I decided for:

if pyproject.is_poetry_project():
    try:
        package_poetry = Factory().create_poetry(pyproject.file.path.parent)
    except RuntimeError:
        package_poetry = None

    if package_poetry is not None:
        ...

@finswimmer finswimmer requested a review from radoering August 22, 2022 04:25
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants