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

Added script file feature #40

Merged
merged 3 commits into from
May 6, 2021
Merged

Added script file feature #40

merged 3 commits into from
May 6, 2021

Conversation

peterdeme
Copy link
Contributor

@peterdeme peterdeme commented Jun 29, 2020

Resolves: python-poetry#241

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

Feature: ability to add script files to distribution packages

This one is a long overdue, see: python-poetry/poetry#241 (June 2018)

The implementation is based on the official specification by @abn python-poetry/poetry#2310

This is a very basic feature of setup.py See here: docs.

Note: this is not the same as entry_point! This setuptools feature basically just copies over the given files to the virtualenv's bin folder so that it'll be available on the $PATH and you can simply invoke it.

Example

[tools.poetry]
package = "my_package"

[tools.poetry.scripts]
migration = { source = "bin/run_db_migrations.sh", type = "file" }
setup_script = { source = "bin/run_setup_script.sh" } # type can be omitted!

Then:

poetry install
poetry run sh run_db_migrations.sh
poetry run sh run_setup_script.sh

Testing

  • I added a couple of automated tests
  • I tested this manually with one of our repositories and it worked

@yajo
Copy link
Contributor

yajo commented Jul 1, 2020

So, will these scripts be be available when installing the app? Or only in dev mode?

@peterdeme
Copy link
Contributor Author

@yajo when installing the app. See “scripts” in setup.py

@finswimmer finswimmer requested a review from a team July 1, 2020 19:45
@muppetjones
Copy link

muppetjones commented Jul 2, 2020

Trying to push for poetry at my company, and this functionality is critical. Glad it's finally getting in!
EDIT: Just to confirm -- the scripts will be added to the path will not require poetry to run, e.g., $ run_setup_script.sh. It looks that way based on the code, but it's unclear from the examples.

@peterdeme
Copy link
Contributor Author

peterdeme commented Jul 2, 2020

@muppetjones
Well, if you activate the virtualenv (or dont use virtualenv at all), you don’t need poetry run prefix. It works exactly the same way as setup.py’s scripts section.

@muppetjones
Copy link

Perfect. That's exactly what I needed. Thanks!
Once this is merged, is there an estimate to when it'll be available in a release?

poetry/core/json/schemas/poetry-schema.json Outdated Show resolved Hide resolved
poetry/core/masonry/builders/builder.py Outdated Show resolved Hide resolved
poetry/core/masonry/builders/builder.py Outdated Show resolved Hide resolved
poetry/core/masonry/builders/wheel.py Outdated Show resolved Hide resolved
@peterdeme peterdeme requested a review from kasteph July 17, 2020 09:58
@finswimmer finswimmer requested a review from a team August 3, 2020 07:08
@peterdeme
Copy link
Contributor Author

I managed to fix the failing build with a poetry lock command. It threw some error (max recursion) during the installation part 🤷‍♂️

@peterdeme
Copy link
Contributor Author

Hi. Can this be looked at please?

@abn
Copy link
Member

abn commented Sep 4, 2020

@peterdeme thanks for pinging, I haven't looked at this one yet but will do so right after we finish up the 1.1.0 release. Apologies for not getting to this earlier.

@abn abn self-requested a review September 4, 2020 20:05
@peterdeme
Copy link
Contributor Author

peterdeme commented Sep 8, 2020

I had merge conflicts, so I rebased my branch. I also squashed my two commits into one.

@roxchkplusony
Copy link
Contributor

@abn did you mean poetry-core 1.1.0 or poetry 1.1.0? Since poetry 1.1.0 went out on Oct 1 and is on 1.1.4 now, would it be a good time to look into this again?

@abn
Copy link
Member

abn commented Nov 2, 2020

@roxchkplusony I can appreciate that this is an important feature for you. But please do understand that this project is maintained by a small group of volunteers. This means that we do not always get the amount of time we would like to invest into the project. And other priorities take precedence, for example bug fix releases.

Please rest assured that this is still on my rather long list of things to get to. And we appreciate your patience ;)

@roxchkplusony
Copy link
Contributor

Of course @abn, don't mean to pressure you. Just wanted clarity on what commitment/estimate you were making, and if it still held. Besides, what does it take to join that inner circle of a "small group of volunteers", anyhow?

@abn
Copy link
Member

abn commented Nov 5, 2020

Just wanted clarity on what commitment/estimate you were making, and if it still held.

I am not sure keeping me "accountable" for time frame is appropriate in this context, obviously I do not work for you. :) Maybe that was not your intent, maybe you simply wanted to know if it is still something on my radar - and as I said before it very much is.

Besides, what does it take to join that inner circle of a "small group of volunteers", anyhow?

I am unsure what you are implying by "inner circle" here, the maintainers do not encourage such a notion. Discussions typically happen on the discord server or on open issues.

You volunteer as with any open source project, start by contributing. If you want to be a maintainer, all I can suggest is to keep contributing long term, interact with the community respectfully and help triage issues. Speaking from my own experience, I contributed for over a year before I was invited to be a maintainer. That said, if you are curious about this, lets discuss this on the discord server and not pollute the discussion relevant to this change.

@peterdeme
Copy link
Contributor Author

peterdeme commented Dec 3, 2020

Fun fact: I opened my first PR for this feature more then a year ago. 🙂
I'm losing hope.

@justinmayer
Copy link

@peterdeme: As an open-source contributor, I have also felt discouragement when PR review takes a long time. As a maintainer, on the other hand, I often don't have enough free time to review PRs as quickly as I would like. So I have seen this from both perspectives and understand them well. Rest assured that folks are doing the best they can and will review your submission as soon as they free up sufficient time.

In the interim, .github/workflows/tests.yml doesn't exist any longer, so I think you'll need to rebase your changes.

Also, I see you have checked the [✓] Updated documentation for changed code checkbox, but I don't see any documentation about the new feature in this PR. That would seem important, since otherwise how will users know that this feature exists?

@peterdeme
Copy link
Contributor Author

@justinmayer thanks for the feedback. Sure, I understand that people do this in their free time and I respect it. One years still feels too long.

  • tests.yml - sure, I'll do that in a minute
  • docs - my previous PR to the main repo included docs. But as far as I know, poetry-core doesn't have documentation, does it?

@justinmayer
Copy link

Ah, yes… Good point. I forgot that this repository has been separated from the one containing the documentation. I imagine the PR templates should be updated to account for that.

Copy link

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

"No newline at end of file": Not sure if these were intended or not, but might want to have a look to make sure.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
justinmayer
justinmayer previously approved these changes Dec 11, 2020
Copy link

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

Many thanks for all your work on this useful enhancement, Peter. 👍

Copy link
Member

@abn abn left a comment

Choose a reason for hiding this comment

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

🥳 LGTM

@abn abn merged commit 8b1cc5e into python-poetry:master May 6, 2021
abn added a commit to abn/poetry-core that referenced this pull request May 7, 2021
This change refactors implementation and tests relating to script file
specifications.

Relates-to: python-poetry#40
Relates-to: python-poetry#241
abn added a commit to abn/poetry-core that referenced this pull request May 7, 2021
This change refactors implementation and tests relating to script file
specifications.

Relates-to: python-poetry#40
Relates-to: python-poetry#241
abn added a commit to abn/poetry-core that referenced this pull request May 7, 2021
This change refactors implementation and tests relating to script file
specifications.

Relates-to: python-poetry#40
Relates-to: python-poetry#241
abn added a commit to abn/poetry-core that referenced this pull request May 7, 2021
This change refactors implementation and tests relating to script file
specifications.

Relates-to: python-poetry#40
Relates-to: python-poetry#241
@miigotu
Copy link

miigotu commented May 7, 2021

Thank you! This is a big win, I'll be working on more plugins now that poetry works well for packaging my project.

@hcoona
Copy link

hcoona commented Sep 22, 2021

May I know how to use this feature now? I found no document about it.

I'm using poetry v1.1.7.

I tried to add the following section but failed to run poetry install.

[tool.poetry.scripts]
gen_clang_syntax_check = {source = "cmd/gen_clang_syntax_check.py", type = "file"}

The error message is shown as following:

Installing dependencies from lock file

No dependencies to install or update

Installing the current project: bmq_dev_support (0.1.0)
  NonExistentKey

  'Key "extras" does not exist.'

  at ~/.local/lib/python3.9/site-packages/tomlkit/container.py:553 in __getitem__
      549│             key = Key(key)
      550│ 
      551│         idx = self._map.get(key, None)
      552│         if idx is None:
    → 553│             raise NonExistentKey(key)
      554│ 
      555│         if isinstance(idx, tuple):
      556│             # The item we are getting is an out of order table
      557│             # so we need a proxy to retrieve the proper objects

It was still failling if I added an empty extra section:

[tool.poetry.extras]

@vchrombie
Copy link

vchrombie commented Sep 22, 2021

May I know how to use this feature now? I found no document about it.

I think the PR is merged but the feature is not yet released in any of the releases. Maybe the maintainers are planning to release this in the 1.2.x versions, not sure tho.

Edit

[tool.poetry.scripts]
gen_clang_syntax_check = {source = "cmd/gen_clang_syntax_check.py", type = "file"}

Since it is a python file, you can directly use the scripts configuration.

@hcoona
Copy link

hcoona commented Sep 22, 2021

Got it, it's poetry-core v1.1.0a5 not poetry v1.1.0, so poetry v1.1.7 didn't contain this feature.

My script file is running with if __name__ == '__main__' pattern & containing args flags. It seems that the script configuration only support running specific method?

@mshafer-NI
Copy link

mshafer-NI commented Sep 22, 2021

@hcoona, yes the current script tag only supports a specific method (that takes no args).

What I usually do is:

def main(argv=None):
    if argv=None:
        argv = sys.argv[1:]
   ... # main program

...

if __name__ == "__main__":
    main()

with:

[tool.poetry.scripts]
script-foo = "foo.__main__:main"

this allows for:
"python -m foo", "poetry run script-foo" to both access "main"
and allows for my module (foo in example) to be imported and called via "foo.main([list of args])"

Hope this is helpful.

<edit - adding some examples using this new option as well>
With the feature in this PR, this could look like (based on tests/fixtures/complete.toml):

script-foo = "foo.__main__:main"
sample_shscript = { reference = "script-files/sample_script.sh", type= "file" }

where "sample_shscript " uses the new options to call "script-files/sample_script.sh"

glumia added a commit to glumia/pylaprof that referenced this pull request Mar 17, 2022
Unfortunately the 'scripts' feature of setuptools[^1] hasn't made its
way into Poetry yet (it has already been merged[^2] but not yet
released).

For the moment we'll need to keep the scripts directory inside
pylaprof (although I wanted to avoid it) and install pylaprof-merge
as an entry point[^3].

[^1]: https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html#the-scripts-keyword-argument
[^2]: python-poetry/poetry-core#40
[^3]: https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html#the-console-scripts-entry-point
@muxator
Copy link

muxator commented Aug 12, 2022

@hcoona, yes the current script tag only supports a specific method (that takes no args).

What I usually do is:

def main(argv=None):
    if argv=None:
        argv = sys.argv[1:]
   ... # main program

...

if __name__ == "__main__":
    main()

with:

[tool.poetry.scripts]
script-foo = "foo.__main__:main"

@mshafer-NI (or anyone who knows), do you happen to know if there is a way of supporting exit codes from main()?

Basically, given a slight variation on your example program, where main() returns an int instead of None:

def main(argv=None) -> int:
    if argv=None:
        argv = sys.argv[1:]
    return 1

Is there a way of having the installed script relaying main()'s return value as exit code?
I have seen this pattern in many installed python packages, I'd want to use it too, but I fail too see how to do it with poetry.

if __name__ == '__main__':
    sys.exit(yourmodule.main())

@mshafer-NI
Copy link

@muxator - I've solved this two different ways

  1. script-foo = "foo.__main__:main", main and main calls sys.exit on error (__main__:main isn't part of my api, so exiting here is fine)
  2. using click and click.abort (docs) on error

@raxod502
Copy link

raxod502 commented May 14, 2023

Did this feature get removed? In Poetry 1.4.2,

[tool.poetry.scripts]
somebinary = {source = "somebinary", type = "file"}

yields the error:

The Poetry configuration is invalid:
  - [scripts.somebinary] {'source': 'somebinary', 'type': 'file'} is not valid under any of the given schemas

There is still documentation at https://python-poetry.org/docs/pyproject#scripts that says this key exists but the examples there don't cover the usage here.

Update: By reading the jsonschema in the PR implementation, I found that source was replaced with reference, so this at least doesn't crash Poetry:

[tool.poetry.scripts]
somebinary = {reference = "somebinary", type = "file"}

It still doesn't do anything (nothing ends up in the virtualenv bin directory).

@keriksson-rosenqvist
Copy link

Does this actually work? I can't seem to get it to run.

I've set this in my pyproject.toml

[tool.poetry.scripts]
my-script = {reference = "path/to/my-script.sh", type = "file"}

poetry install runs fine but when I try to use the script I get the following:

$ poetry run my-script
Warning: 'my-script' is an entry point defined in pyproject.toml, but it's not installed as a script. You may get improper `sys.argv[0]`.

The support to run uninstalled scripts will be removed in a future release.

Run `poetry install` to resolve and get rid of this message.


'callable'

What am I doing wrong and could the Poetry documentation be updated with some examples for different script types?

@wohali
Copy link

wohali commented Jan 22, 2024

This PR doesn't give us what we want. See python-poetry/poetry#2310 (comment) for a detailed explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for scripts