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

poetry install generates invalid python code for console scrips dependent on extras #8899

Closed
4 tasks done
couling opened this issue Jan 23, 2024 · 13 comments · Fixed by #8900
Closed
4 tasks done

poetry install generates invalid python code for console scrips dependent on extras #8899

couling opened this issue Jan 23, 2024 · 13 comments · Fixed by #8900
Labels
kind/bug Something isn't working as expected

Comments

@couling
Copy link
Contributor

couling commented Jan 23, 2024

  • Poetry version: 1.7.1
  • Python version: 3.11.14
  • OS version and name: MacOS
  • pyproject.toml: Documented example under scripts. Eg:
[tool.poetry]
name = "poetry-example"
version = "0.1.0"
description = ""
authors = []
readme = "README.md"

[tool.poetry.dependencies]
python = "^3.10"

# Documented code
[tool.poetry.scripts]
devtest = { reference = "poetry_example:test.run_tests", extras = ["test"], type = "console" }

[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"
  • I am on the latest stable Poetry version, installed using a recommended method.
  • I have searched the issues of this repo and believe that this is not a duplicate.
  • I have consulted the FAQ and blog for any relevant entries or release notes.
  • If an exception occurs when executing a command, I executed it again in debug mode (-vvv option) and have included the output below.

Issue

poetry install generates invalid python for console scrips dependent on extras. If you create a project with the documented toml example and then try to run the script:

% poetry install
Installing dependencies from lock file

Installing the current project: poetry-example (0.1.0)

% source .venv/bin/activate

% devtest
Traceback (most recent call last):
  File "/Users/philip/Documents/Development/scratch/venv/bin/devtest", line 6, in <module>
    sys.exit(test.run_tests[test]())
TypeError: 'function' object is not subscriptable

It seems the generated script is wrong. Eg:

#!/Users/philip/Documents/Development/scratch/venv/bin/python
import sys
from poetry_example import test

if __name__ == '__main__':
    sys.exit(test.run_tests[test]())

This should have been:

    sys.exit(test.run_tests())
@couling couling added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Jan 23, 2024
@dimbleby
Copy link
Contributor

Cf #5297 and #6892 (comment)

Which is to say that you probably don't want to do this...

@couling
Copy link
Contributor Author

couling commented Jan 23, 2024

@dimbleby This is an explicitly documented feature in Poetry. It should work, or be removed from documentation.

scripts

...

To specify a script that depends on an extra, you may provide an entry as an inline table:

[tool.poetry.scripts]
 devtest = { reference = "mypackage:test.run_tests", extras = ["test"], type = "console" }

That is: adding extras = ["foo"] should not result in broken python code generation for the script unless extras ceases to be documented. Better to ignore extras altogether than write a broken python file.

@dimbleby
Copy link
Contributor

dimbleby commented Jan 23, 2024

sure, I'm not saying that everything is awesome. I'm saying that - per the linked comment - the feature you are using is specifically anti-recommended in the python packaging eco-system.

Therefore:

  • regardless of poetry's broken support for it, you probably don't want to use it (eg other installers might or might not do what you hope for with your package)

  • IMO a better direction of travel would be to remove support altogether rather than try to make it work

    • though I don't much care either way about your specific pull request, if you're able to keep it small enough then whatever

@couling
Copy link
Contributor Author

couling commented Jan 23, 2024

@dimbleby you missed my point, I understood yours.

You are flagging a very important issue and I don't want it to get lost (again) in an obscure discussion. I've taken the liberty of creating #8902 so that the documentation aspect can be properly addressed. I'll leave it to your good judgement whether or not a third is required to cover the possible deprecation and removal of the feature entirely.

I'd prefer to keep discussion on this issue to the very narrow topic of ensuring that documented examples work.

@dimbleby
Copy link
Contributor

Since we know that scripts and extras currently simply do not work together - per this and the linked issue - we can reasonably judge that no-one is using them. So I don't see any reason for deprecation, just remove the broken thing.

Actually that is a reason to prefer not to take your merge request making them work a little better - it is easy to take away something that was obviously not working anyway, a little harder to take away something that sort-of works.

@couling
Copy link
Contributor Author

couling commented Jan 24, 2024

@dimbleby You are missing the obviousness of me being here raising this issue: I'm using them. And the author of #5297 was using them. It's reasonable to assume we're not alone. You may have missed (this comment):

I can reproduce this with Poetry 1.1.14 but no longer with the latest version in master. Seems to be fixed.

This feature worked when I migrated the project from setuptools to poetry. It has since stopped working. This is not a feature that has never worked.

Actually that is a reason to prefer not to take your merge request making them work a little better - it is easy to take away something that was obviously not working anyway, a little harder to take away something that sort-of works.

People are using it. It's going to be hard to remove it.

Actually, please never use this logic for documented features. Always assume that someone found a way to use it and follow best practice: Deprecate in documentation, release notes, and console warnings; then delete in a later release.

Making features evaporate, making existing pyproject.toml files suddenly work differently without warning really really damages trust in Poetry as a product.

@dimbleby
Copy link
Contributor

dimbleby commented Jan 24, 2024

if you are using this feature, presumably it is with rather an old version of poetry? My understanding is that this has been broken at least since late 2022 (#6892) and perhaps much longer (#3431)

if that's so then: for sure it looks like it was an accident - but this feature has already been unavailable in poetry for longer than any deprecation period would likely have been.

(By the way, per #6892 (comment) - I don't think #5297 ever was fixed at all)

I see very little value in repairing this if the intention is then to remove it.

@couling
Copy link
Contributor Author

couling commented Jan 24, 2024

@dimbleby You've really misunderstood the situation here. People are using it!. It's worked in the past, it's documented. And only one aspect of it's behaviour is broken. Not the whole feature is broken.

but this feature has already been unavailable in poetry for longer than any deprecation period would likely have been

There's been no release note, and no documentation to remove this feature. So for those of us who don't re-install poetry every 5 minutes and those of us with projects that are locked, shelved for a while and brought back, there's been absolutely no warning at all.

Please note that only one part of this feature is broken. poetry install is broken. poetry build works absolutely fine with this feature and has never been broken AFAIK. So library maintainers (like me) may be totally unaware that the feature doesn't work with poetry install when it works just fine with poetry build.

Again. Please never follow the logic you are using. Please always follow best practice and deprecate first.

@dimbleby
Copy link
Contributor

please say what version of poetry you are using where this works - I tried as far back as 1.1.14 without success.

I reiterate that I don't very much care about your pull request either way, it's small enough not to worry about. My preference remains not to bother - on the grounds that it's the wrong direction of travel and poetry install has been broken here since forever anyway - but I really didn't want to get sucked into an argument about it.

no objections to the removal of scripts-with-extras as used by poetry build being a longer slower process. I see you have an issue open already repeating my point that this is anti-recommended in the broader ecosystem. No doubt a merge request starting that deprecation/removal process would be welcome.

@couling
Copy link
Contributor Author

couling commented Jan 24, 2024

@dimbleby I'm done arguing this. You've wasted too much of my time already. This is a trivial bug with a trivial fix. The feature has been been used a lot throughout our software estate. Discussing this with other senior dev coleagues, it really worries us that Poetry isn't maintained in a way that deprecates before it deletes features.

We primarily use this feature for it's effect on poetry build and the wheels generated. The use-case of poetry install has never been a primary use-case for us but there are devs who use. It has been a real annoyance that it doesn't work despite having worked in the past.

To my mind it makes no sense at all, to have poetry build have significant differences to poetry install. That's just making poetry a lot less useful for library maintainers. Treating the poetry build result as a separate feature to poetry install makes no sense to me at all. They are the same feature executed in two different ways.

@dimbleby
Copy link
Contributor

I think we are talking past each other, here are the points from my last in more compact form

  • please say what version of poetry you are using where this works
  • I don't care about your pull request either way
  • I've no objection at all to a longer-slower deprecation of the broader scripts-with-extras feature

all the best

@couling
Copy link
Contributor Author

couling commented Jan 24, 2024

I don't recall which version. It's been, very approximately, a year since I tested this originally. It might have been more or less. I only know that it appeared to work just fine when I first used it and that a colleague recently flagged it was now broken and I've followed up with issue report and PR.

Copy link

This issue 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
@abn abn removed the status/triage This issue needs to be triaged label Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants