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.

Sign up for GitHub

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

[RFC] [PoC] [WIP] Implement dynamic (SCM) version loader #672

Closed
wants to merge 3 commits into from
Closed

[RFC] [PoC] [WIP] Implement dynamic (SCM) version loader #672

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d603188
Add setuptools-scm(-git-archive) as extra deps
webknjaz Nov 25, 2018
856e326
Get version using an arbitraty executable
webknjaz Nov 25, 2018
daf93d6
Use SCM-based version in the Poetry itself
webknjaz Nov 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions poetry/packages/project_package.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,23 @@

class ProjectPackage(Package):
def __init__(self, name, version, pretty_version=None):
if isinstance(version, str) and version.startswith('attr:'):
Copy link

Choose a reason for hiding this comment

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

The init code would look a little cleaner by moving this code into a new function.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, it's just a PoC.

version_pkg, _, version_attr = (
version[6:].strip().rpartition('.')
Copy link

Choose a reason for hiding this comment

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

Should "attr:function_name" be allowed, without a space after the colon?

Copy link
Author

Choose a reason for hiding this comment

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

Why not? It's harmless and it's how it works in setuptools.

)
if not version_pkg:
version_pkg = 'builtins'
version_getter = getattr(
__import__(version_pkg, fromlist=(version_pkg, )),
version_attr,
)
version = version_getter()
Copy link

Choose a reason for hiding this comment

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

People may want to import a variable instead of a function. The setuptools_scm documentation shows an example in the programmatic usage section https://github.com/pypa/setuptools_scm#programmatic-usage
Checking if the attribute is callable would allow for this. https://docs.python.org/3/library/functions.html#callable

Copy link
Author

Choose a reason for hiding this comment

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

That's different. Also the callable may as well return that constant.

Copy link
Author

Choose a reason for hiding this comment

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

Constant declaration is already supported by setting it in pyproject.toml. There should be only one way to do it.

Copy link

Choose a reason for hiding this comment

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

The variable is dynamically determined; I do not think this is already supported by pyproject.toml. I think adding a small function wrapper that returns a variable is an acceptable thing. But a small if check for whether or not it is callable would support both. Tiny quality of life thing that saves writing a tiny function for people who copy/paste from setuptools_scm docs.

Copy link
Author

Choose a reason for hiding this comment

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

You're suggesting to put import-time evaluation in place, which is exactly what doomed setuptools. And now because of backwards compat they cannot fix this.
That's why I'm -1 here.
If one wants to access the installed version info from their code we could provide them a helper function to do so but let's not do import-time version detection. After all, app needs the version retrieved from pkg_resources in installed mode, it shouldn't invoke version-guessing flows in this case.

Copy link

Choose a reason for hiding this comment

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

What is the difference in what is accomplished between a variable that holds the version, and a function that evaluates to a variable that holds the version? I was suggesting this:

if callable(version_getter):
    version = version_getter()
else:
    version = version_getter

Copy link
Author

Choose a reason for hiding this comment

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

The difference is that it encourages users to have

version = blah_blah()

called on during the module import, which is bad.

if version is None:
raise RuntimeError(
'The version getter callable must return a string '
'or a Version instance'
Copy link

Choose a reason for hiding this comment

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

The message here does not match what the conditional is checking.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this should be fixed.

)

super(ProjectPackage, self).__init__(name, version, pretty_version)

self.build = None
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.poetry]
name = "poetry"
version = "0.12.8"
version = "attr: setuptools_scm.get_version"
Copy link

Choose a reason for hiding this comment

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

would there be a way to enable setuptools_scm to provide the version more directly?

Copy link
Author

Choose a reason for hiding this comment

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

Currently, version arg is read unconditionally + I didn't notice any hooks in poetry

Copy link
Author

Choose a reason for hiding this comment

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

Of course calling setuptools_scm could be hardcoded in python source, but that's a question about flexibility

Copy link

Choose a reason for hiding this comment

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

What is meant by this question? It looks to me like the setuptools_scm library is directly responsible for providing the version in this example.

Copy link
Author

@webknjaz webknjaz Nov 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@kevinkjt2000 This example only shows that users can refer to any callable they want. setuptools_scm has been chosen as a common example but any function will work. That's flexibility.
OTOH @RonnyPfannschmidt seems to want smth like use_version scm = True to work without having to refer to the function explicitly.

Copy link

Choose a reason for hiding this comment

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

@kevinkjt2000 for example in the setuptools integration we trigger the integration and the file finder via the setuptools plugin system

for the poetry integration something similar could be thinkable

Copy link

Choose a reason for hiding this comment

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

I'm a mediocre dev at best and can't intelligently comment, but I would prefer to pass a callable to the config versus offering any integrations. That seems to offer the most flexibility and least complexity in my book.

Copy link

Choose a reason for hiding this comment

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

I may have a lack of understanding here, but to trigger the integration and the file finder via the setuptools plugin system is that not what the get_version function does?

Copy link
Author

Choose a reason for hiding this comment

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

@kevinkjt2000 setuptools has entry_point-based hook system. Other packages can specify their entry points with those name to "register" their hook implementations and setuptools then automatically knows it can use them. Example: https://github.com/pypa/setuptools_scm/blob/master/setup.py#L69

description = "Python dependency management and packaging made easy."
authors = [
"Sébastien Eustace <[email protected]>"
Expand Down