jrnl-org / jrnl

Collect your thoughts and notes without leaving the command line.
https://jrnl.sh
GNU General Public License v3.0
6.49k stars 523 forks source link

Archlinux packaging issues #1009

Closed suderio closed 4 years ago

suderio commented 4 years ago

Bug Report

Thanks for your time, please keep reading before closing as OS packaging problem, just trying to figure out a solution.

Environment

Current Behavior

System upgrade fails due to jrnl dependency being listed as python-cryptography<3.0

Expected Behavior

No problems upgrading, since (AFAIK) there is no (relevant) breaking changes in this library. Tested (loosely) and it worked.

Repro Steps

Other Information

The dependency is listed as "cryptography = "^2.7" in pyproject.toml, which seems suspiciously like something poetry would generate. Now, before anything, if there is a reason to limit cryptography version to <3.0 as this would, please close this issue: the solution would be to change OS packaging to use pipx, as discussed by the maintainer here.

Otherwise, just relaxing this constraint would help us poor arch users. I saw the discussion about loosing up dependency restrictions in some other arch issue, I agree it should be made with caution. On the other hand, when dealing with packages such as this one, the arbitrary constraint will keep bug fixes and security enhancements out of the build, and, well, one can go both ways. Poetry's behavior is understandable, and in this case there are many breaking changes in cryptography 3.0. Version number default semantics justifies it. So, it is a matter of opinion, but an upper limit seems more reasonable when there is a known problem and not as a prophylactic.

I am just giving some rationalizations here. The real reason is I don't like this behavior, and it has been a pain more than a solution. Feel free to ignore. Also, there is a non trivial chance I am missing something here.

Workaround

Uninstall jrnl and reinstall using pipx. Yes, this is the recommended way by the docs. See, I like python, but its dependency hell makes me think it should be named Nachash. :grin:

wren commented 4 years ago

Hi!

Thanks for filing this issue. We're happy to help with the update. I've done some testing, and it looks like the cryptography update didn't break any of the functionality we use it for. I'm happy to have a larger discussion about how best to support arch users, but I'll focus on getting the patch out for the immediate problem first.

wren commented 4 years ago

Welp, it looks like I spoke too soon. Cryptography 3.0 works on my system, but it breaks on Windows according to our test suite.

I'm not sure if that's means it broke on all Windows systems, or just our test pipeline, but we'll need to investigate it before updating the dependency. Sorry!

wren commented 4 years ago

Ok, as promised, I'm here for a wider discussion about how to best support arch users.

So, I totally agree with you that Python has had some pretty serious dependency management problems. In my opinion, many of those problems (at least from a user's point-of-view) stem from sharing global dependencies across multiple programs. And it seems that the AUR package is inadvertently recreating the same situation for arch users. While sharing global dependencies mostly works when are focused on a single distro, the situation starts to get complicated when you factor in other distros, and quickly becomes untenable when you consider in multiple OSes. And that's not even considering multiple versions of Python, and the fact that Python itself has multiple distros, each with its own idiosyncrasies, and multiple versions of each distro, and so on and so forth.

Because of all of this, we can't both relax our dependency requirements, and guarantee a working jrnl install. By the very nature of versioning, jrnl would inevitably break for more and more users as time went on. And I get that probably sounds all theoretical, and preventative, and probably kind of paranoid. But it's already happened to us multiple times. Our homebrew package (for MacOS) was broken for a solid 3 months while we tried to sort out a Python version problem that could've been prevented by more restrictive dependency declarations, and our Windows package has had more of those same types of problems than I can count. This practice unfortunately comes from some hard-earned lessons.

On the bright side, this is exactly why tools like Poetry and Pipx exist. Poetry (and similar tools) lets us as developers make a exact tree of dependencies that we have tested and know works. Pipx installs those exact dependencies into an isolated environment and completely sidesteps the shared global dependency problem. This is one of the reasons why we changed our recommended installation method to Pipx.

That all being said, I want to be clear that we want to support arch users as much as we can. I also definitely see the utility in users being able to install jrnl with the package manager that they're used to (that's why we have a homebrew formula, and have investigated options like choco for Windows). I'm very intrigued by your mention (and in the AUR comments) about using Pipx as a build method for AUR. Am I interpretting that correctly? I didn't know that was an option. If it is, I would strongly recommend updating the formula to do this. I'd be happy to work with whomever and provide some upstream help if that's a deciding factor. While I recognize that this would be more up-front work, I think it would go a long way towards guaranteeing the long-term stability of the AUR package.

What do you think?

suderio commented 4 years ago

Yes, this is a race between two global dependency trees. The arch one gets away with it if its packages don't try to do the same. It is particularly painful with python.

I understand these decisions are usually trade-offs.

Yes, poetry does a good job, I am just skeptic this little detail, the "major version breaks api", is rigorously followed across developer landscape - and about its relevance. But that is a pet peeve born from a different problem.

On the aur packaging, I really can't say for sure. I started reading about it to see if I could help somehow, so not a guru. But it seems possible so far.

StanczakDominik commented 4 years ago

I probably wasn't clear in the aur comments, but: I have absolutely no clue how AUR packages interact with pipx. From what I understand about that model and from what little I know about AUR, they wouldn't really work well together.

I do think it is much simpler to just cut the AUR package and tell people, just use pipx for this. Yes, it's another package manager you have to use on Arch - oh well, such is life.

There were a couple of packages I had to add to the pipx package to get it to work, though... I posted them in the AUR comments. I'll check back and post them back here.

ghost commented 4 years ago

Hi, I'm the current AUR maintainer. Sorry for checking in so late, exams and stuff.

I agree with everything that has been said.

If someone has a foolproof way of using pipx in AUR packages, I will gladly hand over but for now i think axing the AUR package is the most sane approach as much as using an additional package manager feels annoying.

suderio commented 4 years ago

There were a couple of packages I had to add to the pipx package to get it to work, though... I posted them in the AUR comments. I'll check back and post them back here.

Right, forgot to mention that:

pipx install jrnl
pipx runpip jrnl install dbus-python packaging

This worked for me. Just pipx install jrnl gives me an error, missing packaging or something.

StanczakDominik commented 4 years ago

Oh yeah, forgot to add those. Thanks!

StanczakDominik commented 4 years ago

To sum up the issue, I think it's fine to remove the AUR package and just install it via pipx on Arch. I don't think there's any (visible) disagreement at this point.

There has been an AUR comment about removing the pinning of dependencies via a pkgbuild patch that might be an interesting read: https://aur.archlinux.org/packages/jrnl/#comment-758821

I still think it's simpler to just embrace pipx here then maintain a set of patches that will probably break on each release.

At the same time, the two packages described above probably need to be added to dependencies as well. Once that's done, we can probably close the issue.

wren commented 4 years ago

Hi, everyone, I'm back today. Had to go dark yesterday due to some personal stuff that came up. Writing up some responses to everything in this thread shortly.

eli-schwartz commented 4 years ago

Welp, it looks like I spoke too soon. Cryptography 3.0 works on my system, but it breaks on Windows according to our test suite.

I'm not sure if that's means it broke on all Windows systems, or just our test pipeline, but we'll need to investigate it before updating the dependency. Sorry!

The linked PR is failing the tests on Windows due to permission denied errors when trying to create and remove files in %userprofile%\AppData\Local\Temp. The call site is "pip tried and failed to install cffi==1.14.1 on top of an old version of cffi".

I'm finding it difficult to conceptually imagine something less likely to be a legitimate cryptography>=3.0 OS-level incompatibility. Might as well claim the software is incompatible with Windows, because the Windows CI lost power due to a lightning strike.

That's kind of the definition of "unrelated CI failure" and I think it's pretty bad to reject updates to pinned security software based on it. But as I've previously mentioned, you should not be pinning versions.

I probably wasn't clear in the aur comments, but: I have absolutely no clue how AUR packages interact with pipx.

They don't, so don't use it.

I totally agree with you that Python has had some pretty serious dependency management problems.

Python does indeed have some pretty serious dependency management problems. From my perspective as a Linux distribution maintainer who keeps dealing with these problems, the top two problems are "pinned versions exist" and "poetry exists".

The entire problem here is that you're using requirements to declare that jrnl only works with one pinned version of each dependency, rather than using lockfiles to declare the versions which are tested. I notice you're also still pinning the version of the Olson tz database data files, despite the version of the pytz module not being based in any way shape or form on the code itself.

The correct solution is to require minimum versions of your dependencies based on what users might have available, maximum versions only when you have reason to believe a dependency is more likely to break than to work (or you know something is broken and can't fix it yet), and again, use lockfiles to communicate the tested set which users should prefer to install.

The semi-correct solution is for linux distros to patch jrnl and remove all version pinning, since it's been proven to cause issues without solving problems.

In the long run, I'll merely observe that the average distro package is from 2014, before the version pinning was introduced: https://repology.org/project/jrnl/versions It's nice to know this is how you treat distros that attempt to provide modern versions of your software. ;)

That is, of course, the worst-case non-solution for any program...

wren commented 4 years ago

It looks like everyone is okay using pipx for now. Thanks for all of your reporting, troubleshooting, and patience.

@flocomkoko No worries about timing, and thanks for chiming in. I really appreciate the work you've done so far to keep that package up to date. Thank you!

@suderio @StanczakDominik The missing packaging dep was completely our fault and a known problem in v2.4.4 (#1010). It's fixed in v2.4.5. Sorry about that! If you could give that version a try and report back if you still have any troubles installing with pipx on arch, I would really appreciate it.

Also, with pipx in place as a solid alternative, we're still open to working on a native installation option for arch. After some research into arch packaging, it looks like there are some guidelines for Python packages. I did some quick testing using this docker image (note: I don't know how representative the image is of a real arch desktop), and I think we might be able to get something workable in place if we can get a committed, arch tester or three. Any volunteers?

ghost commented 4 years ago

Hi, thank you for the kind words.

I'm sorry, I have orphaned the AUR package.

A few very unhelpful comments in the AUR comments were the tipping point and combined with the issues we are facing concerning libraries I no longer have the energy to maintain a package I am not even using.

I hope you can work with a new maintainer if someone decides to pick up the package although I think the pipx route is the only approach with any hope since dependency pinning is pretty incompatible with the rolling release approach of Arch Linux. An "arch trusted user" in the AUR comments called your dependency pinning "insane" and strongly recommended patching out all version checks, something I heavily disagree with.

I'm sorry to see this go, I enjoyed working with you. You have been a very helpful "upstream" so to say.

ghost commented 4 years ago

I'm not sure if that's means it broke on all Windows systems, or just our test pipeline, but we'll need to investigate it before updating the dependency

@wren It looks to me your CI jobs for Windows failed, because of some Windows permissions weirdness. I've seen error like that a few times and have no real idea why it happens, but installing packages inside freshly created venv in directory 100% owned and controlled by user seems to always work.

eli-schwartz commented 4 years ago

The official docker image is https://hub.docker.com/_/archlinux

eli-schwartz commented 4 years ago

@maln0ir I did mention this:

The linked PR is failing the tests on Windows due to permission denied errors when trying to create and remove files in %userprofile%\AppData\Local\Temp. The call site is "pip tried and failed to install cffi==1.14.1 on top of an old version of cffi".

wren commented 4 years ago

@flocomkoko I'm really sorry you had to deal with that. It's been a pleasure working with you, too. I sincerely wish you luck on whatever you do next, and I hope that your experience with such unhelpful comments doesn't scare you away from the open source community for good.

@maln0ir Yup, it does look like you're right about the CI problems. We're working on sorting them out, and we'll have a patch out as soon as we can properly test across all our supported platforms. And thank you for taking a look; it's always helpful to have an extra set of eyes!

@eli-schwartz Yup, that's the official image; thanks for pointing it out. I gave it a shot earlier, but it was a bit too stripped down for the quick testing I was looking for. I was looking for a representation of an average desktop, and the one I used seemed relatively useful for that. If and when we integrate this into a proper testing pipeline, we would absolutely use the official image.


Also, with all of this packaging talk, I'm reminded that homebrew works on linux now, too (as linuxbrew). And assuming linuxbrew works like homebrew, it'll deal well with overlapping dependencies very well without relying on global packages. Homebrew is our recommended install method for macos already (we helped update the homebrew package for jrnl v2), so linuxbrew could provide a reliable package for those not wanting to use the python-specific pipx.

eli-schwartz commented 4 years ago

What is your recommendation for people who want to rely on their well-known package manager (pacman, xbps, nix, dnf, guix) and are seeing by default the 2014 release (1.9.8) which didn't use version pinning?

P.S. I understand criticism is often seen as disruptive, but it would still probably be a good idea to try to answer the concerns I raised.

wren commented 4 years ago

I just talked it over with @micahellison, and here's what we've got:

The package managers we have officially maintained and tested are pipx and brew, so those are still our recommendation (for now). As I mentioned above, we definitely see the utility in users being able to install jrnl with the package manager that they're used to. We honestly didn't realize that jrnl was available through other package managers (none of those listing are from us), so it's good to learn about those now (thank you for the repology link, @eli-schwartz).

We definitely want to work with the community to provide (tested and stable) packages through those channels. That is going to take some time and effort, and--as I mentioned above--the biggest help we can get is from testers (at least until we can integrate these packages into our pipeline). Even if we don't get any testers, we'll be working towards getting those packages updated.

wren commented 4 years ago

P.S. I understand criticism is often seen as disruptive, but it would still probably be a good idea to try to answer the concerns I raised.

I marked your comment as disruptive not because it contained criticism, but because we received complaints about it.

This is a good a time as any to remind everyone to please review our Code of Conduct, and to please keep criticism constructive, and all comments professional.

wren commented 4 years ago

Also, I think (?) that the packaging issues are resolved for pipx and/or brew on arch since we added the missing dependencies. If I could get some confirmation on that, I would really appreciate it. Otherwise, I'll let this issue sit for a while then close it up if I don't hear back.

Look for the updated package manager stuff in other issues as we go through that work.

Thanks, everyone!

StanczakDominik commented 4 years ago

Also, I think (?) that the packaging issues are resolved for pipx and/or brew on arch since we added the missing dependencies. If I could get some confirmation on that, I would really appreciate it.

I just checked; you added packaging, right? dbus-python is still a necessity on Arch, at the very least. You still have to do pipx runpip jrnl install dbus-python to get it to work.

To be explicit: I installed the current development version via pipx install git+https://github.com/jrnl-org/jrnl@develop.

StanczakDominik commented 4 years ago

Ah, looking through it I guess this might be a bit more complex, as https://github.com/jrnl-org/jrnl/issues/478#issuecomment-286543647 seems to suggest that dbus-python is an optional dependency for keyring to store auth data in the system keyring (KDEWallet, in my case).

wren commented 4 years ago

@StanczakDominik That's a very good point. That looks like a dep that's only needed on some systems. I think we can add it for everyone for now, and then worry about build-specific dependencies later.

What do you think about this, @micahellison ?

micahellison commented 4 years ago

I'm a little hesitant, since I don't know a ton about system keyrings or how Python interfaces with them, and the documentation for dbus-python starts off with a warning about how problematic it is. We've also already been burned by folks adding in a deprecated version of a missing module (the readline issue documented in #1015).

But if there's no risk to users, I'm all for it. Is there a way we can demonstrate that?

wren commented 4 years ago

@micahellison According to their readme, keyring only relies on the dbus package on linux (https://pypi.org/project/keyring/). So, as long as our keyring tests pass on other platforms, I think we're in the clear.

micahellison commented 4 years ago

OK, that works for me.

wren commented 4 years ago

@StanczakDominik If you have some time, would you mind testing the brew release to see if it needs a similar dependency update?

On a related note: the next (pip) release of jrnl will have the updated dependencies (or you can get them now by running from source).

StanczakDominik commented 4 years ago

I can try running brew on Arch, but for a complete picture you'd probably need someone on Mac to test that.

StanczakDominik commented 4 years ago

Yeah, it's complaining about python-dbus. I'd expect packaging crashing as well.

wren commented 4 years ago

@StanczakDominik Thanks for testing that out.

I've done some local and docker testing for including python-dbus as a dependency, and it looks like the package python-dbus will fail quite thoroughly if the system doesn't have the relevant dbus packages. I think pipx runpip jrnl install dbus-python is going to be the best way to go until we have an arch-specific pacakge ready. Sorry, and thanks again for all the help!

I do think we can update the docs, and add a more useful error message with this info for users that run into the problem, though. So, I'll make a new issue to deal with that (since the original arch packaging issues are now irrelevant).

wren commented 4 years ago

Ok, sounds like there's no more follow up here so I'm closing this one up. Please watch #1029 for more updates for the relevant issues.