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

setup.py shouldn't import the package it installs #823

Closed
greut opened this issue May 12, 2019 · 17 comments · Fixed by #824
Closed

setup.py shouldn't import the package it installs #823

greut opened this issue May 12, 2019 · 17 comments · Fixed by #824
Labels
bug Bug report. core Related to the core parser code. needs-decision A decision needs to be made regarding request.

Comments

@greut
Copy link

greut commented May 12, 2019

There is a 🐔 and 🥚 problem regarding how setup.py works. the version number should be parsed from the text file rather than imported via python mechanism.

    File "setup.py", line 26, in <module>
      from markdown import __version__, __version_info__
  ModuleNotFoundError: No module named 'markdown'
@greut
Copy link
Author

greut commented May 12, 2019

cf #791

@mitya57
Copy link
Collaborator

mitya57 commented May 12, 2019

The same thing now happens in Travis BTW, see https://travis-ci.org/Python-Markdown/markdown/builds/531020063.

I am not yet sure what causes it, though — setup.py imports Markdown but Markdown does not import setup.py so there is no cyclic dependency.

@facelessuser
Copy link
Collaborator

I guess there is one way to be sure, we can perform an experiment. We can move the version stuff to a separate file, and then manually import just that file directly.

@mitya57
Copy link
Collaborator

mitya57 commented May 12, 2019

I will try to do that a bit later.

Also I don't know if it will be useful or not, but modern setuptools supports specifying the version in setup.cfg in declarative format:

version = attr: markdown.__version__

or

version = file: VERSION.txt

The former worked for me some time ago, need to check again. Upd: still works in https://travis-ci.org/retext-project/pymarkups/builds/502474546 although I do not use Tox.

@facelessuser
Copy link
Collaborator

If you can declare it like you mentioned, and have it exist in one place, maybe that is preferable.

@mitya57
Copy link
Collaborator

mitya57 commented May 12, 2019

I was able to fix Travis tests by setting PYTHONPATH to the current directory: https://github.com/mitya57/Python-Markdown/commit/291a71239a245371a0ad455ec6a279b02814c235. Strangely, I was not able to reproduce the original problem locally, even when using tox.

@greut How exactly are you calling setup.py? Can you check if setting PYTHONPATH=$(pwd) fixes the issue for you too?

@greut
Copy link
Author

greut commented May 13, 2019

@mitya57 I'm installing it via Requirements-Builder and the git+https URL. Pip fails into the _vendor/pep517/_in_process.py@get_requires_for_build_wheel. Something like, pypa/pip#6163

Is the project fully PEP517 compliant?

edit strangely django/django installs just fine and it seems to perform same kind of 🐔 and 🥚. So it should be possible.

@greut
Copy link
Author

greut commented May 13, 2019

btw python3 -m pep517.check fails

@waylan
Copy link
Member

waylan commented May 13, 2019

This is definitely a PEP517 issue, which makes this a duplicate of #791. However, I'm closing that as this better identifies the issue.

Under Build Environment, PEP517 states:

A build backend MUST be prepared to function in any environment which meets the above criteria. In particular, it MUST NOT assume that it has access to any packages except those that are present in the stdlib, or that are explicitly declared as build-requirements.

Note that the cwd is not listed among the locations which we can assume to have access to (this is the change which is breaking things. We have always been able to make that assumption in the past). In a later subsection, In-Tree Build Backends, there is a provision defined for giving the build environment access to the source tree:

Projects can specify that their backend code is hosted in-tree by including the backend-path key in pyproject.toml. This key contains a list of directories, which the frontend will add to the start of sys.path when loading the backend, and running the backend hooks.

Of course, the source code for the project is not technically part of the build environment backend, in Python-Markdown's case, but we could use it that way if we want to leave the version identification code as-is.

@mitya57 demonstrated how we could define the version in setup.cfg in a comment above. Or we could define the version in setup.py itself. However those are using the old system which Python seems to be leaving behind.

I think we should probably explore adopting the new system and use pyproject.toml. However, that means defining the version in the pyproject.toml file. If we want to have the version accessible from markdown.__version__, then we would likely need to import disutils from the markdown lib and retrieve the disutls registered version of markdown, which we could assign to markdown.__version__. Unless there is some other way to do that, that I'm not aware of.

Regardless of what solution we use, we want to only ever define the version in one location. Statically defining the version in multiple locations is only asking for them to get out of sync. I'm having a hard time believing the new system doesn't have a simple API for this already, but I haven't found one.

@waylan waylan added bug Bug report. core Related to the core parser code. needs-decision A decision needs to be made regarding request. labels May 13, 2019
@waylan
Copy link
Member

waylan commented May 13, 2019

Interestingly, when I had first learned of pyproject.toml, I had considered moving markdown to using it. That would have made sense for the 3.0 release (or even the 3.1 release). But I decided against it for the reason that I dislike the way versions are defined. Our current setup requires that we define the individual pieces of the __version_info__ tuple and those pieces are programmatically assembled into a valid version number. Personally, I never remember the specifics of how those pieces come together (especially for alpha, beta, rc and the like). Any change would loose that. But if people can't install the lib, then it doesn't matter what format the version number is in, valid or not.

@mitya57
Copy link
Collaborator

mitya57 commented May 13, 2019

In a later subsection, In-Tree Build Backends, there is a provision defined for giving the build environment access to the source tree

I think in-tree backends were not meant to be used this way. The PEP mentions only two use cases: backends themselves and wrappers around existing backends where the wrapper is too project-specific. I think our case does not fall into any of these two.

I think we should probably explore adopting the new system and use pyproject.toml. However, that means defining the version in the pyproject.toml file.

Can you give a pointer to some documentation about this, or a project that specifies a version in pyproject.toml? I have not heard about this yet.

There is flit build system that allows one to specify metadata in pyproject.toml but it does not allow to specify version there…

If we want to have the version accessible from markdown.__version__, then we would likely need to import disutils from the markdown lib and retrieve the disutls registered version of markdown, which we could assign to markdown.__version__.

By disutils do you mean distutils? If yes, then I think it does not support getting the version of installed package. One can use pkg_resources to get the version, but pkg_resources has a big performance impact (see #790).

@mitya57
Copy link
Collaborator

mitya57 commented May 13, 2019

Can you give a pointer to some documentation about this, or a project that specifies a version in pyproject.toml? I have not heard about this yet.

I performed some search and found a build system called poetry that indeed does allow one to specify version in pyproject.toml. Did you mean this build system?

@mitya57
Copy link
Collaborator

mitya57 commented May 13, 2019

I have just created #824 which is quite ugly but should fix this.

It looks like the imported version information is used in three places in setup.py: the version itself, the development status (for classifiers) and the download URL (which is currently broken though). So I decided to not switch to setup.cfg right now because it will mean losing the development status classifier.

@greut Can you please try to install from git+https://github.com/mitya57/Python-Markdown.git and see if your problem is fixed?

@waylan
Copy link
Member

waylan commented May 13, 2019

Can you give a pointer to some documentation about this, or a project that specifies a version in pyproject.toml?

I have to admit I was writing that based on memory. Perhaps I remembered incorrectly, but I have the impression that pyproject.toml is intended as a way to replace the need for any setup.py script. If you can't define the version in pyproject.toml then how do you do so? Or do you pass the version to the build command from the command line (which is a terrible solution IMO)? My overall takeway last time I looked was that using pyproject.toml is as bad as, if not worse than, using setup.py and/or setup.cfg.

By disutils do you mean distutils? If yes, then I think it does not support getting the version of installed package. One can use pkg_resources to get the version, but pkg_resources has a big performance impact

Ah yes, I meant pkg_resources. And yes, I realize it is not a desirable solution. But if we define the version number in the package config rather than the code, it is the only way to make the version available in the code.

My real point here is that to avoid the need to use pkg_resources to fetch the version, we need to continue to define the version in the source code. And yes, I realize in-tree backends were not meant to be used this way. I even acknowledged as much. But we're essentially in a catch-22 situation here.

I see two options:

  1. Embrace the new stuff and use pkg_resources to populate markdown.__version__.
  2. Cling to the old-school stuff and continually make minor hacks to keep things working.

@mitya57
Copy link
Collaborator

mitya57 commented May 13, 2019

I have to admit I was writing that based on memory. Perhaps I remembered incorrectly, but I have the impression that pyproject.toml is intended as a way to replace the need for any setup.py script.

Most Python projects are still using a setup.py script, though it can be as minimal as this if you have all the needed metadata in setup.cfg.

As I pointed out, there are build systems (flit, poetry) that allow you to specify metadata directly in pyproject.toml, but they are not mainstream yet.

I see two options:

  1. Embrace the new stuff and use pkg_resources to populate markdown.__version__.
  2. Cling to the old-school stuff and continually make minor hacks to keep things working.

A possible variant 3 may be using setuptools-scm which gets the version from Git (or other Vcs). I am not using it in my projects, but I think it is getting quite popular nowadays.

Anyway, my pull request #824 follows option 2 as it is the smallest change that fixes this bug (hopefully, waiting for the submitter’s feedback). I am not sure if “continually” making hacks will be needed — Python ecosystem changes quite slowly but we'll see.

@waylan
Copy link
Member

waylan commented May 13, 2019

If you can't define the version in pyproject.toml then how do you do so?

So I looked at flit and right on the first page is states that you define the version in your sourcecode with version = .... I was curious so I looked at the source code and by initial fear at first seemed to be confirmed, but then resolved. As the comments explain:

# Attempt to extract our docstring & version by parsing our target's
# AST, falling back to an import if that fails. This allows us to
# build without necessarily requiring that our built package's
# requirements are installed.

Of course, for us parsing the AST will fail, but the fallback which imports would work fine. My issue is that the import has to be the fallback. We are stuck always using the fallback. On the plus side, flit also does a version_check which normalizes the version, so maybe that would remove the need for us to compile our version manually.

Most Python projects are still using a setup.py script, though it can be as minimal as this if you have all the needed metadata in setup.cfg.

I never saw the point of that. If you need the setup.py script anyway, then why bother with the setup.cfg?

As I pointed out, there are build systems (flit, poetry) that allow you to specify metadata directly in pyproject.toml, but they are not mainstream yet.

Exactly. But if we are going to switch, it seems that using the new system would make sense. I'm not convinced that we should make that change now, but it is at least worth considering. As you note, these changes happen slowly, so it will likely be some time before we look at this again. On the other hand, when people starting moving from distutils to setuptools, we took the wait-and-see approach and then found ourselves as one of the few remaining holdouts. While users initially thanked us for holding off, as time progressed we got more and more complaints that we hadn't adopted the new normal.

A possible variant 3 may be using setuptools-scm which gets the version from Git (or other Vcs).

While I understand the appeal of this, I don't see it as a something I want to use. I find it weird that the version is defined in a tag (meta-data of the VCS), not in any of the files for the project. And it requires the call to pkg_resources.

@mitya57
Copy link
Collaborator

mitya57 commented May 14, 2019

I never saw the point of that. If you need the setup.py script anyway, then why bother with the setup.cfg?

You usually have both anyway, so it's a matter of taste where to write stuff. Some configuration is easier to write in setup.cfg than in setup.py. For example,

long_description = file: README

is more simple than

with open(os.path.join(os.path.dirname(__file__), 'README')) as readme_file:
    long_description = readme_file.read()

setup(...
   long_description=long_description,
...)

I'm not convinced that we should make that change now, but it is at least worth considering.

I agree. Maybe let’s look how the Python landscape changes in the coming year(s).

I find it weird that the version is defined in a tag (meta-data of the VCS), not in any of the files for the project.

I share the same opinion! I just mentioned that tool to add more choice, just in case you were not aware of it.

waylan pushed a commit that referenced this issue May 15, 2019
 Add pep517check environment to tox

 Split version info into a separate file, load it using importlib

Fixes #823.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report. core Related to the core parser code. needs-decision A decision needs to be made regarding request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants