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

Drop pbr #1991

Merged
merged 4 commits into from
Dec 20, 2021
Merged

Drop pbr #1991

merged 4 commits into from
Dec 20, 2021

Conversation

eli-schwartz
Copy link
Collaborator

This migrates off of the final two features of pbr that were still in use after #1883

  • setuptools data files now natively supports globbing. Require a recent enough version of setuptools during the build.
  • Switch from pbr's git versioning to the popular setuptools_scm.
    • maintained by PyPA
    • has dedicated scope to do one thing (SCM versioning) and do it well
    • fixes the bug where pbr reimplemented git describe by manually parsing commit logs and got the wrong answer
    • future versions of setuptools_scm will transparently support git archive generated tarballs

@eli-schwartz
Copy link
Collaborator Author

CI passes in my fork...

@mlouielu
Copy link
Collaborator

CI passes in my fork...

I believe is cause by our latest main failed, not your PR.

I'll take time to review it today.

@mlouielu mlouielu self-requested a review December 17, 2021 01:10
@eli-schwartz
Copy link
Collaborator Author

Yeah I don't understand the "pipenv" mess there at all.

I'm sure it must be possible to simply install the tools you need and run them... that is what I do for my projects...

@Davidy22
Copy link
Collaborator

I presume it's for handling python projects with conflicting versions of the same dependency, previous maintainer's been voicing some support for poetry for a bit, I presume because of things like this

@eli-schwartz
Copy link
Collaborator Author

Is that a general comment about python, or a comment about this project specifically?

If it's about this project specifically, I will be very surprised, because guake lists no dependencies installable by either pipenv OR poetry. Because guake basically does not depend on anything other than the GObject Introspection bindings to various C libraries.

In fact, that's exactly why you had a problem in that other PR you linked to this PR. Someone apparently discovered that guake does not actually work when installed with pipenv, because it needs python-gobject, something that distros provide, and vte, libnotify, libkeybinder, libutempter, libwnck... also things that distros provide.

That may be why guake describes on its installation docs:

Always prefer using your package manager to install guake.

Anyway, pygobject is not really in the business of causing conflicts in the version of anything. It's a gnome project, gnome prefers to create versioned installations of their software when changing API/ABI (this is why, for example, gtk2 / gtk3 / gtk4 can ALL be installed on the same system, next to each other).

@eli-schwartz
Copy link
Collaborator Author

(As a general rule of thumb, I'm extremely skeptical of anyone claiming "conflicting versions of the same dependency", as it almost always means that one or both projects had an overly-rigid dependency specification on an exact version of its dependencies. 99% of the time distros patch out those dependency specifications and it works flawlessly. The remaining 1% of the time is a project bug for not supporting new versions of a dependency, but that is after all a project bug... also it is actually pretty rare.)

@Davidy22
Copy link
Collaborator

He actually opened an issue #1878, and additionally commented about it various times. I haven't dived into doing the env manager switch myself because it usually doesn't get in my way as long as I don't touch it.

I've run into issues where a library deprecated/removed/moved something in a newer version, and an ancient package that I wanted to use needed the deprecated function while another package needed a new feature and I'd have to go about updating the old package myself to have both work at the same time. This scenario wasn't really helped by environments because these were both being used in the same project, but it generally saves from the annoying chance that a new/old version of a dependency installed for something you downloaded and ran pip install -r requirements on doesn't rug pull anything else you have installed and make you have to do a round of debugging when you come back to the other program that was silently broken.

The non-pip dependencies were a hefty chunk of the head-banging I was doing with trying to fix the pypi build. I did have half a mind to just give up on trying to update that package, given the additional distro packages that need to be installed anyways, but it was something I wanted to do when I first saw the pypi version stay at 3.7 when 3.8 launched.

For this specific issue, some searching tells me pipenv regressed after the pipenv version we had pinned before. This convinces me that I should approve #1992, which reverts #1989 which was force merged for some reason even though it still failed CI. I am aware of pipenv's reputation for shuffling things around on a whim between versions, which makes bumping its version number labor intensive and which might be the reason why an alternative has been proposed before.

@eli-schwartz
Copy link
Collaborator Author

Given another PR was landed to fix CI, this PR can be fixed once I rebase it, give me a couple minutes. :)

I suspect that re-triggering the CI will just cause it to build the original merge prediction, rather than the CI doing something useful like re-merging the master branch in... so it is going to be stuck continually re-trying whichever commit hash was baked into the CI instance at the moment of the PR's creation.

In setuptools 2712 setuptools learned to natively
support globbing data files, and the pbr setting to do the exact same thing is
not needed anymore. Pin the minimum build requirement to ensure the correct
version is used.
There is no need to ship both .po and .mo files.
setuptools_scm is a standard and robust tool for dynamically getting
version numbers from e.g. git; among its features is the ability to
write the results to a python module that the application can then
import from. This removes the overhead of large modules which introspect
the dist-info to get the version, in favor of importing a frozen string.
@mlouielu
Copy link
Collaborator

Given another PR was landed to fix CI, this PR can be fixed once I rebase it, give me a couple minutes. :)

I suspect that re-triggering the CI will just cause it to build the original merge prediction, rather than the CI doing something useful like re-merging the master branch in... so it is going to be stuck continually re-trying whichever commit hash was baked into the CI instance at the moment of the PR's creation.

Thanks, my manual re-run seems not using the latest master branch : (

@eli-schwartz
Copy link
Collaborator Author

Yay, the CI seems to be happy now. :)

@mlouielu
Copy link
Collaborator

Before merging this, we should add unit-test to test version, and maybe check it with test-pypi for packaging.

@mlouielu
Copy link
Collaborator

For package manager, maybe we should discuss it in another thread.

I think at least we should abandon one (requirements.txt v.s. pipenv) to eliminate duplicate changes, and then discuss which package manager should we migrate to or not.

@eli-schwartz
Copy link
Collaborator Author

What particular attribute of the version do you want to test? That it has a version? Or... ?

(There were no tests to test the version produced by pbr, so I'm not sure what you want to test.)

@Davidy22
Copy link
Collaborator

Davidy22 commented Dec 19, 2021

Oh, I do remember people with python versions < 3.8 had issues with Guake crashing inside guake_version() in __init__.py which I readded a pbr fallback for, and since this PR modifies that function that's probably the function being referred to here.

@mlouielu
Copy link
Collaborator

What particular attribute of the version do you want to test? That it has a version? Or... ?

(There were no tests to test the version produced by pbr, so I'm not sure what you want to test.)

In my mind have two:

  1. For guake itself: guake --version should have the version result
  2. For packaging: @eli-schwartz from your perspective, will this work for pypi and distro packaging? (e.g. Archlinux)

@eli-schwartz
Copy link
Collaborator Author

setuptools_scm is actually a lot more reliable than importlib.resources, because guake/_version.py is generated if it does not exist, every time setup.py is run (not just setup.py install or setup.py bdist_wheel). In contrast, getting the version from importlib.resources only works once a dist-info has been created (typically this means installing guake).

So, it should be able to be run locally without installing anything, e.g.

# does it work when immediately cloning?
$ python -m guake.main --version
Traceback (most recent call last):
  File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/home/eschwartz/git/guake/guake/main.py", line 634, in <module>
    exec_main()
  File "/home/eschwartz/git/guake/guake/main.py", line 620, in exec_main
    if not main():
  File "/home/eschwartz/git/guake/guake/main.py", line 422, in main
    print(f"Guake Terminal: {guake_version()}")
  File "/home/eschwartz/git/guake/guake/__init__.py", line 25, in guake_version
    from ._version import version
ModuleNotFoundError: No module named 'guake._version'

# but now it works
$ python setup.py --version
3.8.2.dev26+gcd2d902f
$ python -m guake.main --version
Guake Terminal: 3.8.2.dev26+gcd2d902f
VTE: 0.66.2
VTE runtime: 0.66.2
Gtk: 3.24.30

i.e. running any setuptools command, including the one that just prints the package number, causes setuptools_scm to calculate the version number and save it to the file.

For packaging on PyPI, setuptools_scm will detect that it isn't being run in a git repo, and look at the PKG-INFO file that setuptools creates as part of its sdist management. For distro packaging, setuptools_scm works fine and distros are more familiar with how to work with it than pbr. :)

Future versions of setuptools_scm will also support version info from a github archive tarball (I know this because I submitted the patches to the git project, and notified the setuptools_scm project about the new feature in order to prepare for its inclusion in setuptools_scm).

When that happens I will submit another PR here to hook that up.

@Davidy22
Copy link
Collaborator

Added a short release note file with a little footnote to let package maintainers know about the change here, once CI passes, which it should, I'll merge since this looks fine

@Davidy22 Davidy22 merged commit 6dc23f3 into Guake:master Dec 20, 2021
@mlouielu
Copy link
Collaborator

Thanks @eli-schwartz for the PR and your detailed comment!

@eli-schwartz
Copy link
Collaborator Author

Thanks!

@eli-schwartz eli-schwartz deleted the drop-pbr branch December 20, 2021 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants