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

Python 3.10 support #1270

Closed
musicinmybrain opened this issue Jun 24, 2021 · 5 comments · Fixed by #1271
Closed

Python 3.10 support #1270

musicinmybrain opened this issue Jun 24, 2021 · 5 comments · Fixed by #1271
Labels
enhancement New feature or request

Comments

@musicinmybrain
Copy link
Contributor

Feature Request

Use Case/Motivation

Python 3.10.0 beta 3 is available for testing, and Fedora Linux has already integrated Python 3.10 in the development version (Rawhide). Fedora 35 will release with Python 3.10 in the fall, around the same time that Python 3.10 final is available.

As the maintainer of the jrnl package in Fedora, I’m loosening the Python version restriction in pyproject.toml downstream.

With a recent update to asteval, jrnl now builds, the tests pass (pytest and behave), and the executable passes a basic interactive “smoke test” at the command line.

Since it seems there are likely no changes required other than pyproject.toml, please consider officially supporting Python 3.10 at your earliest convenience.

Example Usage

(not applicable)

Other information

pytest results:

=============================== warnings summary ===============================
../../../../usr/lib/python3.10/site-packages/ansiwrap/core.py:6
  /usr/lib/python3.10/site-packages/ansiwrap/core.py:6: DeprecationWarning: the imp module is deprecated in favour of importlib and slated for removal in Python 3.12; see the module's documentation for alternative uses
    import imp

-- Docs: https://docs.pytest.org/en/stable/warnings.html
======================== 96 passed, 1 warning in 0.87s =========================

behave results:

16 features passed, 0 failed, 0 skipped
305 scenarios passed, 0 failed, 5 skipped
1683 steps passed, 0 failed, 27 skipped, 0 undefined
Took 0m27.708s
@musicinmybrain musicinmybrain added 🆕 New! enhancement New feature or request labels Jun 24, 2021
@micahellison
Copy link
Member

Hi there @musicinmybrain, thanks for testing and filing this, and for your work on the Fedora distribution. It's always nice to hear from other maintainers.

I've started a PR on this, though our CI is falling to install dependencies in Python 3.10, so it may take a while to resolve. We'll keep an eye on it during our regular work sessions, and in the meantime, I'd welcome any assistance from anyone interested in troubleshooting the issue.

@wren
Copy link
Member

wren commented Jun 26, 2021

@musicinmybrain Thanks for all the work on this!

I see in the spec that you're also doing some other changes. Should we take a look at those, too? If there are fixes you're having to do, we'd love to get those merged in upstream where hey can benefit everyone. Here are the ones I see (with my questions and notes).

  • Python version
    In order to catch these types of updates sooner (and preferably before they have to be reported), we've made an issue to test the next version of python nightly. This way we'll know when our dependencies update, and be able to act accorindly. We actually used to have this on Travis, but it fell through the cracks when migrating to Github Actions. We'll be putting that back soon. Thanks, again, for pointing this out!

  • Dependencies
    We regularly review dependencies to make sure they're the best they can be, but obviously we don't always get it right. I see you're loosening a few requirements (tzlocal, pyxdg, poetry, and parsedatetime). We had set those versions because they fixed some bugs we had run into (or at least we thought so), but it seems like you haven't been running into any problems.

    Is that right? If so, we should probably revisit the minimum versions we have set for those dependencies. Would you mind talking about how you arrived at the versions you've updated the dependencies to? Is it just the version that's available on Fedora? Or was there something else?

  • Shebangs
    Haha, I think I've been using shebangs wrong my entire life. 😅 It looks like we probably only need one on jrnl/__main__.py. Is that right?

    Also, even though they're being used incorrectly here (and we should fix that), I thought they were mostly harmless when included. So, I'm curious as to why they're being removed here. Is it a precaution? Were they causing an issue of some sort? Is there something else?

  • Man pages
    We should absolutely have man pages. Our help screen has been getting pretty bloated as we've added more features, so, moving some of that out to man pages seems like a good solution. Would it be useful to package the man pages along with jrnl?

Thanks, again, for everything!

@musicinmybrain
Copy link
Contributor Author

musicinmybrain commented Jun 26, 2021

@wren, I appreciate your excellent questions. I’ll follow up here a little later, when I have time to address them more thoroughly.

@musicinmybrain
Copy link
Contributor Author

Would you mind talking about how you arrived at the versions you've updated the dependencies to? Is it just the version that's available on Fedora? Or was there something else?

There isn’t as much being changed as it seems; in the latest spec file, only tzlocal is loosened for Rawhide (the development version of Fedora, which will be branched to become Fedora 35 in the fall). That package is lagging in Fedora, and I should file an issue and/or send a PR to its maintainer requesting an update. Then Fedora 35 will be able to release with a jrnl package that has all of the upstream version specifications intact.

Other adjustments in the spec file are specific to existing stable Fedora releases, and preserve the ability to build the latest package on the two current stable releases of Fedora. It’s nice to be ready in case I need to issue an update due to a major bug or security vulnerability, although both releases currently contain older versions of jrnl and I’m likely to leave them alone if possible. For those releases, it is really just a matter of working with the package versions we already have in Fedora. I only took over maintenance of jrnl in Fedora two months ago, so to some extent the status quo is what it is. I’ll handle any bug reports in the existing releases, but I’m mostly focused on keeping jrnl up-to-date and cleanly packaged going forward.

As I implied above, my hope is to be able to align a recent version of jrnl (hopefully the latest version) with compatible dependency versions by the time each future stable Fedora release occurs, so that I don’t have to adjust pyproject.toml at all. In those cases where I do have to (hopefully temporarily) allow a dependency outside the ranges specified upstream, at least all the tests run as part of the RPM build, so I can still have some confidence that the package is not severely broken.

It’s good to know that you care about this. If I can’t get dependencies aligned by a future Fedora release, but the tests are passing anyway, I’ll be sure to file an issue or PR to try to accommodate a wider version range upstream.


Haha, I think I've been using shebangs wrong my entire life. sweat_smile It looks like we probably only need one on jrnl/main.py. Is that right?

You need a shebang (and the executable bit set) if you plan to run a file as a script, something like:

./foo

In this case, I am not so sure that you need any shebangs. People normally don’t run modules inside a package that way, especially when installed system-wide—who types /usr/lib/python3.9/site-packages/foo/bar.py at the terminal?

In the jrnl package, the only module whose file mode is executable is jrnl/Entry.py; but looking at the source, it is clear that it is only meant to be imported, and it “executing” it has no interesting side effects. (I should submit a PR to change the file permissions so it is no longer executable.)

Meanwhile, the module that is written like a “script” is jrnl/__main__.py, which has the usual if __name___ == "__main___": boilerplate. But it is not executable, so the shebang is not useful here either! Plus, the sensible way to to execute it as a script is to ensure the jrnl/ package directory is in the Python path (perhaps just by cd’ing to the project directory), and do python3 -m jrnl.

Finally, there is the jrnl command that you get when you install the package (with pip, or from a distribution package). That’s generated by

[tool.poetry.scripts]
jrnl = 'jrnl.cli:cli'

in pyproject.toml, and the resulting script will correctly have a shebang line and the executable bit set, looking something like this:

#! /usr/bin/python3 -s
# -*- coding: utf-8 -*-
import re
import sys
from jrnl.cli import cli
if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
    sys.exit(cli())

Also, even though they're being used correctly here (and we should fix that), I thought they were mostly harmless when included. So, I'm curious as to why they're being removed here. Is it a precaution? Were they causing an issue of some sort? Is there something else?

I’d say they are mostly harmless; the worst effect is probably confusion (“was this supposed to be a script? I don’t see anything but class and function definitions!”)

Still, in Fedora we have these packaging guidelines for shebangs, which say “Files which are not installed as executables SHOULD NOT have shebang lines.” You’re right that it’s best to upstream the change wherever possible.

Finally, shebang lines are often written like #!/usr/bin/env python3 or #!/usr/bin/env bash. This case is different—it’s often the right thing to do from an upstream perspective, because it searches the PATH. For example, a user could install Python under $HOME/.local and meddle with their PATH, and #!/usr/bin/env python3 would use that Python. But as a distribution, we don’t want this behavior. Scripts that are installed system-wide should use the system-wide interpreters from the distribution, not whatever is on the user’s PATH. So we have a policy that these shebangs must be adjusted, e.g. to #!/usr/bin/python3, even if the #!/usr/bin/env … shebang still makes sense upstream. The %py3_shebang_fix macro does this—plus, it converts any use of an unversioned python interpreter to python3.

Would it be useful to package the man pages along with jrnl?

Yes, absolutely! That way, you would take responsibility for its quality, and other distributions would be likely to package the man page as well. I have commented on #1274.

@wren
Copy link
Member

wren commented Jul 1, 2021

@musicinmybrain Thank you for the very detailed and informative response! I just wanted to let you know that I've read it and will respond soon. @micahellison and I usually work on jrnl on Saturdays, so we'll go over this then and decide on a course of action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants