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 files support #1504

Closed
wants to merge 3 commits into from
Closed

Added script files support #1504

wants to merge 3 commits into from

Conversation

peterdeme
Copy link

@peterdeme peterdeme commented Oct 26, 2019

Pull Request Check List

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

Feature: script-files section in pyproject.toml

Reasoning

This one is a long overdue, see: #241 (June 2018)

Basically, our company wants to migrate over Poetry but it's impossible since Poetry does not provide a way to add non-Python scripts to the dist package and our migration project is pretty much blocked 😞. 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"
script-files = ["bin/run_db_migrations.sh"]

Then:

poetry install
poetry run sh run_db_migrations.sh

Testing

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

Possible discussion

Naming. Well, I was scratching my head to come up with a decent name. script-files sounds sensible to me, but I can change it if you guys want.

edit.: for some reason the GitHub Actions build on Windows failed for me. Looks like it tried to run the commands in powershell instead of cmd - I don't know why, maybe GitHub changed something this weekend? Anyway, I enforced cmd in the build declaration, the build looks good now.

@peterdeme
Copy link
Author

Can this have some visibility?

@peterdeme
Copy link
Author

@stephsamson - what do you think?

@jleclanche
Copy link
Contributor

I'm not a fan of this approach… There is solid prior art on how to do this in yarn/npm and pipenv.

This is completely intuitive:

[tool.poetry.scripts]
dosomething = "./path/to/dosomething --arg"  # Path relative to pyproject.toml

I also agree with your assessment in this comment that the "scripts" naming for what "scripts" currently is doesn't make sense if it's just to replicate setuptools' entrypoints.

It should either be renamed tool.poetry.entry_points, or tool.poetry.scripts should be able to safely detect whether you're attempting to run a script vs. an entrypoint.

@jleclanche
Copy link
Contributor

Just to clarify the comment above, this is more in response to this PR resolving #241, which I don't think it does; IMO that issue has more to do with poetry's usage as a project manager than a project packager. Being able to include scripts files in the resulting package makes sense, but isn't the whole picture.

@peterdeme
Copy link
Author

peterdeme commented Nov 14, 2019

@jleclanche yeah, I was also confused about the terminology.
I think it's a bad idea to introduce such a breaking change so I did not rename existing functions.

Being able to include scripts files in the resulting package makes sense, but isn't the whole picture.

Care to elaborate? Just to clarify I don't wanna shake the earth with this PR, I just want for our (and obviously many other) companies to enable this feature.

@peterdeme
Copy link
Author

@sdispater opinion?

@finswimmer finswimmer added kind/feature Feature requests/implementations area/build-system Related to PEP 517 packaging (see poetry-core) labels Dec 23, 2019
@elihunter173
Copy link

To avoid changing scripts to entry_point and breaking a bunch of people's projects (although I agree entrypoints makes more sense), I think calling the new functionality tasks is intuitive.

For example, if I have something like below in my pyproject.toml, then I'd expect to be able to do either poetry dosomething or poetry task dosomething to execute the task.

[tool.poetry.tasks]
dosomething = "./path/to/dosomething --arg"  # Path relative to pyproject.toml

@peterdeme
Copy link
Author

peterdeme commented Jan 1, 2020

Nice idea, it makes sense but I’m still a bit confused how is this relevant to this feature. This isn’t about running arbitrary scripts but copying any kind of files (not just executables) to a package.

@danbradham
Copy link

danbradham commented Jan 16, 2020

Glad to see this PR @peterdeme thank you for working on it. I'm not sure why everyone is misunderstanding this PR as well as #241, but hopefully this will push the feature along.

The scripts keyword of setup() allows you to install non-python scripts along with a python package. This should not be confused with running arbitrary development tasks.

I personally feel like tool.poetry.scripts should be renamed for the sake of clarity, but, that seems like a larger discussion / outside of the scope of this PR. The script-files keyword seems fine, I could also see something a bit more explicit like include-scripts working well.

Can @sdispater or any other maintainers weigh in on this? I think there are probably quite a few projects that are unable to use poetry due to this missing feature.

# Conflicts:
#	.github/workflows/main.yml
#	README.md
#	poetry/masonry/builders/builder.py
#	tests/masonry/builders/test_complete.py
@sdispater sdispater mentioned this pull request Jan 31, 2020
2 tasks
@AstraLuma
Copy link

Any motion on this? I just reworked a bunch of stuff in my project to work around #241.

@peterdeme
Copy link
Author

@astronouth7303 the PR’s been open for 4 months. Looks like they don’t have bandwith to review it or it’s a low priority to them

@geckon
Copy link

geckon commented Feb 28, 2020

I'd also love to see attention to this as it's something I think poetry definitely should support and I'd love to use this feature instead of trying to come up with a work-around.

@funkyfuture
Copy link
Contributor

imo poetry shouldn't have invented its own semantics for keywords in the first place and i think the 1.0 release was premature. i don't know how the maintainers employ the versioning scheme as the last patch release broke builds, so if breaking changes are also okay for minor releases, i'd say the current [tool.poetry.scripts] section should be renamed to [tool.poetry.console_scripts] asap and the feature in this pr should handle the [tool.poetry.scripts] section.

@finswimmer
Copy link
Member

i don't know how the maintainers employ the versioning scheme as the last patch release broke builds,

Patch releases shouldn't break anything. So if this really happens it wasn't intended. Could you please open an issue for this or give us a linke to an existing issue?

@funkyfuture
Copy link
Contributor

Could you please open an issue for this or give us a linke to an existing issue?

that was #2107

@sdispater
Copy link
Member

@funkyfuture

i don't know how the maintainers employ the versioning scheme as the last patch release broke builds

Poetry follows semver. And the latest release did not break builds on purpose, it's just that bugs happen, unintended side effects happen. Semver is not a silver bullet against unplanned regressions. While we try our best to not break anything, we are only human and sometimes mistakes happen.

That being said, the Git issue you mentioned should now be fixed in the latest release.

@peterdeme
Copy link
Author

peterdeme commented Mar 11, 2020

@sdispater @finswimmer @stephsamson this has been open for around half year and I barely saw any relevant comments here. Can you guys at least tell me if this is bad or good or something? This would unblock a lot of people/companies plan to migrate to Poetry. We're also a Python-shop at TrueMotion and we can't wait to adopt Poetry more but the lack of this feature blocks us.
Thank you.

@kasteph
Copy link
Member

kasteph commented Mar 29, 2020

Hi @peterdeme,

Thank you for your contribution! This PR is a great one. Although it is not part of the roadmap afaik, I think this should be seriously considered @sdispater.

I also think naming it tasks instead of script-files makes more sense since it’s equivalent to rake/bundle/make tasks.

@AstraLuma
Copy link

THEY"RE NOT PROJECT TASKS.

This defines a new type of installable: Files that should be placed on the $PATH without modification.

@peterdeme
Copy link
Author

@stephsamson , thanks for the feedback. it is not really equivalent with those. As astrobouth said this one just copies a bunch of files to your PATH.

https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html#the-scripts-keyword-argument

@kasteph
Copy link
Member

kasteph commented Mar 30, 2020

@astronouth7303 thanks for the clarification, I was hastily scanning the comments of the PR when I was reviewing a bunch of others and was careless. I won't do this next time and be more careful but typing in caps lock is also unnecessary :)

Regarding the keyword, I think an RFC needs to be opened on whether poetry should be using keywords equivalent to setup.py and would mean breaking changes.

@AstraLuma
Copy link

Sorry, the issue is like 3 miles long because of this confusion.

@kasteph
Copy link
Member

kasteph commented Mar 30, 2020

No worries, I can understand that it's incredibly frustrating. I'll make sure to raise this particular issue once again to the core team.

Copy link
Member

@kasteph kasteph left a comment

Choose a reason for hiding this comment

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

Hi there,

Sorry for getting back late but I've come back with feedback.

The core team has come to an agreement that instead of introducing a new namespace in the pyproject.toml file, that we'd rather have extended inline tables under the tool.poetry.scripts namespace instead. See here.

I'd kindly ask you to make changes in the core repo and I'll make sure to review them ASAP (you can ping me on Discord, my username there is same as here on GitHub).

Thank you for your contribution! 🙌


```toml
[tool.poetry]
script-files = ["contrib/run_db_migration.sh", "bin/healthcheck.py"]
Copy link
Member

Choose a reason for hiding this comment

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

The @python-poetry/core team has decided on going forward with keeping the [tool.poetry.scripts] namespace and using inline tables to have more flexibility and backwards compatibility.

@@ -82,8 +82,15 @@
},
"format": {
"oneOf": [
{"type": "string"},
{"type": "array", "items": {"type": "string"}}
{
Copy link
Member

Choose a reason for hiding this comment

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

This schema will have to change w.r.t. my comment above.

@peterdeme
Copy link
Author

I’ll try to find time in the next few weeks.

@peterdeme
Copy link
Author

Today, I had time to develop this. I opened a new PR and closing this one now.

@stephsamson Can you please take a look at python-poetry/poetry-core#40?

Copy link

github-actions bot commented Mar 1, 2024

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 Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/build-system Related to PEP 517 packaging (see poetry-core) kind/feature Feature requests/implementations
Projects
None yet
Development

Successfully merging this pull request may close these issues.