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

Unable to export a package with a circular dependency #118

Closed
neersighted opened this issue Sep 7, 2022 · 8 comments · Fixed by #121
Closed

Unable to export a package with a circular dependency #118

neersighted opened this issue Sep 7, 2022 · 8 comments · Fixed by #121

Comments

@neersighted
Copy link
Member

neersighted commented Sep 7, 2022

Poetry 1.2.0 cannot export a project with a cyclic dependency. I'm unsure if this is an exporter or a Poetry issue, or what would be involved in fixing it, as I have not dug into it myself yet.

$ pipx install poetry==1.2.0
$ git clone https://github.com/python-poetry/poetry && cd poetry
$ poetry export -f requirements.txt -o requirements.txt

Dependency walk failed at poetry (>=1.2.0b3,<2.0.0)

Ironically, that version constraint comes from this repository itself (poetry = ^1.2.0b3).

@dimbleby
Copy link
Contributor

dimbleby commented Sep 7, 2022

In general there's no issue with the export code encountering dependencies that it has previously seen, the issue is specifically if it encounters the top-level project again later in the tree.

IMO the export code is right to barf on this and the rest of poetry is wrong to tolerate it and the preferred fix would be to remove the cyclic dependency.

But it would surely be possible to cope with this special case, if necessary.

@neersighted
Copy link
Member Author

That was roughly my guess without having looked at the source code. On one hand, I want to break the circular dep. On the other hand, that will be a slightly drawn out process and I would like to be able to export Poetry itself sooner than later.

@brechtm
Copy link
Contributor

brechtm commented Sep 15, 2022

I'm running into the same issue with my package rinohtype, which specifies a default set of font packages to install (e.g. rinoh-typeface-dejavuserif) which in turn depend on rinohtype. I don't think this is necessarily wrong. For example, some of these font packages could in the future require a minimum version of rinohtype, which should be specified.

@dimbleby
Copy link
Contributor

oh, it's definitely wrong!

In your case it sounds as though the font packages should depend on the rinohtype package, but not the other way round. rinohtype doesn't depend on the font packages, you just find it convenient to group them for installation.

(Strongly analogous to the relationship between poetry and this plugin, if I've understood correctly)

if people want to install both the rinohtype and fonts, they can depend on them both. Or, if you want, you could publish a meta-package, rinohtype-with-default-fonts or something, which depended on both.

@dimbleby
Copy link
Contributor

If anyone cares about this enough to submit an MR the fix should be to plumb root.name through get_project_dependency_packages, via get_project_dependencies, and into walk_dependencies.

Then we can go if requirement.name == root_package_name: continue (maybe making a log while we're there).

And a testcase would be good of course.

@brechtm
Copy link
Contributor

brechtm commented Sep 16, 2022

oh, it's definitely wrong!

In your case it sounds as though the font packages should depend on the rinohtype package, but not the other way round. rinohtype doesn't depend on the font packages, you just find it convenient to group them for installation.

rinohtype does depend on the font packages since the default stylesheets included with it make use of them. I don't want to include the fonts in the rinohtype package, because they should be upgradable independently. It also keeps rinohtype upgrades small since the same fonts then do not need to be downloaded again if they do not change.

if people want to install both the rinohtype and fonts, they can depend on them both. Or, if you want, you could publish a meta-package, rinohtype-with-default-fonts or something, which depended on both.

That seems very inconvenient.

Perhaps you can explain why this is wrong? I haven't seen any reasoning to support that claim. Note that this works without issue with an older version of poetry.

@dimbleby
Copy link
Contributor

dimbleby commented Sep 16, 2022

I mean it's obviously wrong to have a circular dependency, you have no bottom turtle!

Anyway I don't care to argue about it, if you want to submit an MR then I've laid out above how to handle this, and earlier comments on this issue indicate that maintainers would be minded to accept it.

brechtm added a commit to brechtm/poetry-plugin-export that referenced this issue Sep 16, 2022
@brechtm
Copy link
Contributor

brechtm commented Sep 16, 2022

I'm not here to argue either. I'm genuinely interested in learning why circular dependencies would be wrong. It is not at all obvious to me, I'm afraid. It just sounds like a hollow dogmatic statement akin to "circular imports are a sign of bad design".

In any case, I'm glad a fix for this would be accepted. Your suggestion does the trick, but I'll need some more time to figure out how to write a test case.

brechtm added a commit to brechtm/poetry-plugin-export that referenced this issue Dec 7, 2022
brechtm added a commit to brechtm/poetry-plugin-export that referenced this issue Dec 7, 2022
brechtm added a commit to brechtm/poetry-plugin-export that referenced this issue Jan 6, 2023
mwalbeck pushed a commit to mwalbeck/docker-python-poetry that referenced this issue Feb 28, 2023
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [poetry](https://python-poetry.org/) ([source](https://github.com/python-poetry/poetry), [changelog](https://python-poetry.org/history/)) | minor | `1.3.2` -> `1.4.0` |

---

### Release Notes

<details>
<summary>python-poetry/poetry</summary>

### [`v1.4.0`](https://github.com/python-poetry/poetry/blob/HEAD/CHANGELOG.md#&#8203;140---2023-02-27)

[Compare Source](python-poetry/poetry@1.3.2...1.4.0)

##### Added

-   **Add a modern installer (`installer.modern-installation`) for faster installation of packages and independence from pip** ([#&#8203;6205](python-poetry/poetry#6205)).
-   Add support for `Private ::` trove classifiers ([#&#8203;7271](python-poetry/poetry#7271)).
-   Add the version of poetry in the `@generated` comment at the beginning of the lock file ([#&#8203;7339](python-poetry/poetry#7339)).
-   Add support for `virtualenvs.prefer-active-python` when running `poetry new` and `poetry init` ([#&#8203;7100](python-poetry/poetry#7100)).

##### Changed

-   **Deprecate the old installer, i.e. setting `experimental.new-installer` to `false`** ([#&#8203;7358](python-poetry/poetry#7358)).
-   Remove unused `platform` field from cached package info and bump the cache version ([#&#8203;7304](python-poetry/poetry#7304)).
-   Extra dependencies of the root project are now sorted in the lock file ([#&#8203;7375](python-poetry/poetry#7375)).
-   Remove upper boundary for `importlib-metadata` dependency ([#&#8203;7434](python-poetry/poetry#7434)).
-   Validate path dependencies during use instead of during construction ([#&#8203;6844](python-poetry/poetry#6844)).
-   Remove the deprecated `repository` modules ([#&#8203;7468](python-poetry/poetry#7468)).

##### Fixed

-   Fix an issue where an unconditional dependency of an extra was not installed in specific environments ([#&#8203;7175](python-poetry/poetry#7175)).
-   Fix an issue where a pre-release of a dependency was chosen even if a stable release fulfilled the constraint ([#&#8203;7225](python-poetry/poetry#7225), [#&#8203;7236](python-poetry/poetry#7236)).
-   Fix an issue where HTTP redirects were not handled correctly during publishing ([#&#8203;7160](python-poetry/poetry#7160)).
-   Fix an issue where `poetry check` did not handle the `-C, --directory` option correctly ([#&#8203;7241](python-poetry/poetry#7241)).
-   Fix an issue where the subdirectory information of a git dependency was not written to the lock file ([#&#8203;7367](python-poetry/poetry#7367)).
-   Fix an issue where the wrong Python version was selected when creating an virtual environment ([#&#8203;7221](python-poetry/poetry#7221)).
-   Fix an issue where packages that should be kept were uninstalled when calling `poetry install --sync` ([#&#8203;7389](python-poetry/poetry#7389)).
-   Fix an issue where an incorrect value was set for `sys.argv[0]` when running installed scripts ([#&#8203;6737](python-poetry/poetry#6737)).
-   Fix an issue where hashes in `direct_url.json` files were not written according to the specification ([#&#8203;7475](python-poetry/poetry#7475)).
-   Fix an issue where poetry commands failed due to special characters in the path of the project or virtual environment ([#&#8203;7471](python-poetry/poetry#7471)).
-   Fix an issue where poetry crashed with a `JSONDecodeError` when running a Python script that produced certain warnings ([#&#8203;6665](python-poetry/poetry#6665)).

##### Docs

-   Add advice on how to maintain a poetry plugin ([#&#8203;6977](python-poetry/poetry#6977)).
-   Update tox examples to comply with the latest tox release ([#&#8203;7341](python-poetry/poetry#7341)).
-   Mention that the `poetry export` can export `constraints.txt` files ([#&#8203;7383](python-poetry/poetry#7383)).
-   Add clarifications for moving configuration files ([#&#8203;6864](python-poetry/poetry#6864)).
-   Mention the different types of exact version specifications ([#&#8203;7503](python-poetry/poetry#7503)).

##### poetry-core ([`1.5.1`](https://github.com/python-poetry/poetry-core/releases/tag/1.5.1))

-   Improve marker handling ([#&#8203;528](python-poetry/poetry-core#528),
    [#&#8203;534](python-poetry/poetry-core#534),
    [#&#8203;530](python-poetry/poetry-core#530),
    [#&#8203;546](python-poetry/poetry-core#546),
    [#&#8203;547](python-poetry/poetry-core#547)).
-   Validate whether dependencies referenced in `extras` are defined in the main dependency group ([#&#8203;542](python-poetry/poetry-core#542)).
-   Poetry no longer generates a `setup.py` file in sdists by default ([#&#8203;318](python-poetry/poetry-core#318)).
-   Fix an issue where trailing newlines were allowed in `tool.poetry.description` ([#&#8203;505](python-poetry/poetry-core#505)).
-   Fix an issue where the name of the data folder in wheels was not normalized ([#&#8203;532](python-poetry/poetry-core#532)).
-   Fix an issue where the order of entries in the RECORD file was not deterministic ([#&#8203;545](python-poetry/poetry-core#545)).
-   Fix an issue where zero padding was not correctly handled in version comparisons ([#&#8203;540](python-poetry/poetry-core#540)).
-   Fix an issue where sdist builds did not support multiple READMEs ([#&#8203;486](python-poetry/poetry-core#486)).

##### poetry-plugin-export ([`^1.3.0`](https://github.com/python-poetry/poetry-plugin-export/releases/tag/1.3.0))

-   Fix an issue where the export failed if there was a circular dependency on the root package ([#&#8203;118](python-poetry/poetry-plugin-export#118)).

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNTIuNSIsInVwZGF0ZWRJblZlciI6IjM0LjE1Mi41In0=-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-python-poetry/pulls/655
Co-authored-by: renovate-bot <[email protected]>
Co-committed-by: renovate-bot <[email protected]>
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.

3 participants