-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Path based scripts #304
Path based scripts #304
Conversation
poetry/masonry/builders/sdist.py
Outdated
if scripts: | ||
before.append( | ||
"scripts = \\\n{}\n".format( | ||
pformat([f"bin/{name}" for name in scripts]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace that f-string with old-style string formatting, so as to support older versions of Python?
Note that while the branch name talks about non-python scripts, this is not fully yet possible with this patch as |
setuptools supports path-base "scripts", which can be included in wheels. I have no idea what this is supposed to have to do with Makefiles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuration syntax and general approach looks good to me. My only blocking concern is the copying to bin
in the sdist (expanded on this in another comment).
result = defaultdict(list) | ||
scripts = {} | ||
|
||
# Scripts -> Entry points | ||
for name, ep in self._poetry.local_config.get("scripts", {}).items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec
might make more sense vs ep
since it can be an "entry point" or a script
@@ -176,6 +175,10 @@ def find_excluded_files(self): # type: () -> list | |||
def dist_info(self): # type: () -> str | |||
return self.dist_info_name(self._package.name, self._meta.version) | |||
|
|||
@property | |||
def data_info(self): # type: () -> str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The directory itself is *.data
, but naming the property data
would probably be confusing. What do you think about naming the methods below make_data_name
and make_dist_info_name
and then naming this method data_name
and changing dist_info
to dist_info_name
?
|
||
_, scripts = self.convert_entry_points() | ||
for name, script in scripts.items(): | ||
to_add.append((Path(script), Path("bin") / name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fail with newer pip which creates and installs a wheel from the source distribution and supports pyproject.toml. Since pyproject.toml
still refers to the original path of the script, it will fail since those would not exist in the sdist.
One approach to fix this may be to include the original file with the original path in the source distribution and then in the generated setup.py we can copy those files to a temporary directory with the correct name (since setuptools does not provide a facility for mapping to a different name in the scripts
key) and then pass those paths in the scripts
list to setup()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also using relative_to
like below would be good since it would reject values like "../other-project/bin/script"
. convert_entry_points
will need to return absolute paths for scripts in order for this to behave as expected.
Any updates on this PR? I have a project that depends on this functionality. Definitely eager to see this introduced! |
Thanks for contributing to poetry! We've got a bit of a backlog of pull requests. I'll be focused on getting the bug fixes through first so it could be a while. Apologies. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Thanks for your contribution! However I am closing this in favor of #1504 since I prefer using a new attribute/section for this purpose rather than using an existing one. Feel free to comment and review in the other PR. |
@sdispater I agree that other PR seems like a better approach. |
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. |
See #241 for details.
Adds support for non-function-based scripts to be included in distributions. The syntax is like this: