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

koreader: init at 2022.05.1 (built from source) #178557

Closed
wants to merge 5 commits into from
Closed

koreader: init at 2022.05.1 (built from source) #178557

wants to merge 5 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 22, 2022

Reviewers: it will be easier to read this PR if you look at each commit individually, rather than smashed together the way the "Files Changed" tab shows them.

The first three commits are trivial; they are included because without them CI will fail. The fourth commit renames the existing koreader expression to koreader-bin without any changes to it. The fifth commit marks the sourceProvenance of koreader-bin.

The sixth commit is where all the interesting stuff is.

Two utility functions, mapAttrsToConcatStringSep and buildLuaRock, near the top of the file are likely to be useful outside of this particular package. They have comments suggesting they might be moved elsewhere. I prefer to do that in separate PRs following this one.

Ancestral WIP-draft was #178320

Description of changes

This expression (koreader/src.nix) builds koreader from source.

The previous koreader expression has been renamed koreader-bin as suggested by @AndersonTorres, and has been moved to koreader/bin.nix.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.

@ghost ghost mentioned this pull request Jun 22, 2022
13 tasks
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jun 22, 2022
@ofborg ofborg bot requested a review from contrun June 22, 2022 11:22
pkgs/applications/misc/koreader/src.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/koreader/src.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/koreader/src.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/koreader/src.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/koreader/src.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/koreader/vendored.nix Show resolved Hide resolved
@ghost ghost requested a review from AndersonTorres June 23, 2022 04:27
@ghost
Copy link
Author

ghost commented Jun 24, 2022

Force-push is to resolve the merge conflict caused by 466c2e3

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

Heroic, I should say.

Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Do we really have to vendor all of these? I'm sure koreader builds fine with any version of most of these dependencies. Using a vendored version has the disadvantage of longer build times and untimely security updates.

pkgs/top-level/all-packages.nix Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jun 24, 2022

Do we really have to vendor all of these? I'm sure koreader builds fine with any version of most of these dependencies.

That is my next project!

However, I think that the default nix-env -iA koreader should always use the upstream-approved versions of koreader's dependencies.

If we diverge from the versions used by upstream, any quirks or bugs that arise from our version-bumps will create "support blowback" for upstream... people will complain to them about weird bugs that they can't reproduce without realizing that it's really our fault for bumping the version of something (say, glib or openssl) without testing its effect on koreader.

My plan is to add a parameter unVendor = [ "pname", "pname", "pname" ... whose default value the largest possible set of dependencies which can be unvendored without breaking the build, and then override that parameter to the empty set (i.e. leave everything vendored) in the koreader expression.

So top-level.nix would look roughly like:

  koreader             = callPackage ../applications/misc/koreader/src.nix { unVendor = []; }
  koreader-unvendored  = callPackage ../applications/misc/koreader/src.nix { }

We should not mislead people into thinking that koreader-unvendored is koreader. I think it is inevitable that koreader-unvendored will become broken quite frequently... I'm not sure I can commit to keeping it unbroken at all times the same way I commit to rapid-response maintainership of koreader/src.nix and vendored.nix.

Using a vendored version has the disadvantage of longer build times and untimely security updates.

Yes, and the shorter build times are the main attraction for me, since I'm about to start hacking on koreader... soon my ebook reader will finally have a "totally ignore the touchscreen" mode like all of my dead-tree books do!

@ghost
Copy link
Author

ghost commented Jun 24, 2022

Heroic, I should say.

:blushes:

@dotlambda
Copy link
Member

However, I think that the default nix-env -iA koreader should always use the upstream-approved versions of koreader's dependencies.

Our default should be secure.
I don't see any reason to use a vendored version of e.g. zstd.

Does koreader have tests that we could use to make sure our versions work well?

@Frenzie
Copy link

Frenzie commented Jun 24, 2022

The main reason we "vendor" e.g. zstd is because the primary target is embedded systems.

It should be perfectly fine to use djvulibre, freetype2, fribidi, giflib, glib (or perhaps rather sdcv; that's just a dependency), harfbuzz, libffi, libiconv, libjpeg-turbo, libpng, libunibreak, possibly luajit (if sufficiently recent), luasec/luasocket (if sufficiently recent), minizip, NOT MuPDF, sdcv, sqlite, zlib, and zstd from the system, probably a few more. NB The *zmq stuff is mesozoic era and incompatible with recent versions.

But yes, if you run the tests you should get a reasonably decent indication.

Doing so would require some time investment in koreader-base. For example by introducing some kind of MINIMAL_DESKTOP variable to say that we're in some kind of Debubuntu/NixOS environment where we want to use the system libraries if available.

@AndersonTorres AndersonTorres mentioned this pull request Jun 25, 2022
13 tasks
@mweinelt mweinelt added 12.approvals: 1 This PR was reviewed and approved by one reputable person and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jun 25, 2022
@ghost
Copy link
Author

ghost commented Jun 26, 2022

Squashed.

Adam Joseph and others added 5 commits June 26, 2022 00:59
This expression (`koreader/default.nix`) builds koreader from source.

The previous koreader expression has been renamed `koreader-bin` as
suggested by @AndersonTorres, and has been moved to koreader/bin.nix.

Co-authored-by: Anderson Torres <[email protected]>
@ghost
Copy link
Author

ghost commented Jun 26, 2022

The main reason we "vendor" e.g. zstd is because the primary target is embedded systems.

This is stretching the word "embedded"... these aren't microcontrollers running an RTOS. All the popular ebook devices (Kindles and Kobos) are full-fledged arm-linux-gnueabi machines -- Linux, glibc, and all. Kindles even have their own glib.so, and koreader uses it. AFAICT "kindle build" of koreader just sets CC=arm-linux-gnueabi-${CC} and decides which touchscreen+eink driver gets built.

It should be perfectly fine to use ...

Thanks for clarifying. Does this statement apply to ereader builds as well? Because those are the ones that really matter. Being able to nix build pkgsCross.arm32-linux.koreader.kindle and rsync it to my device is a goal.

Also, curious, what is different about MuPDF? Koreader seems to backport a huge amount of material to mupdf-1.13.0; did something awful happen in the following release?

But yes, if you run the tests you should get a reasonably decent indication.

It is not clear that getting the tests to run inside the nix sandbox is feasible.

They expect network access, only partly in order to download more lua packages that aren't used during the build. It's extremely tedious to work on them inside the sandbox because the tests run after the entire build has finished, and the build takes a long time, and AFAICT nix doesn't have good support for restarting partially-completed sandboxed builds. Modularizing the build into separate derivations would need to come first.

some kind of MINIMAL_DESKTOP variable to say that we're in some kind of Debubuntu/NixOS environment

This is slightly funny since I don't run NixOS and am frequently told that my workstation isn't a "desktop" because it omits certain software ;) But I guess all terminology is loaded in some way.

@Frenzie
Copy link

Frenzie commented Jun 27, 2022

Thanks for clarifying. Does this statement apply to ereader builds as well? Because those are the ones that really matter. Being able to nix build pkgsCross.arm32-linux.koreader.kindle and rsync it to my device is a goal.

If pkgsCross.arm32-linux.koreader.kindle truly means Kindle and not generic ARM, sure. However, then the thing you said about "support blowback" applies. We have put a lot of time and effort into https://github.com/koreader/koxtoolchain and without that you might very well see all manner of mystery issues, which wouldn't be the case on any kind of halfway regular Linux system. "full-fledged arm-linux-gnueabi machines -- Linux, glibc, and all" describes a fairy tale utopia, or at least it sounds to me like it implies that when you have those things available they aren't horribly broken in all manner of fun ways that you'd never see on a proper Linux system.

To some extent that even applies to OpenWrt (I guess you wouldn't like its homepage), but there saying it's a full-fledged Linux is a lot more accurate because it's actually trying to be fairly generic, normal, and proper. It's not largely broken outside of the very few specific use cases that matter to the manufacturer. Use whichever word you like, but you have to approach it as if it were an embedded system, hic sunt dracones. If you approach it as if it were a regular Linux system since it looks like one you'll burn your hand.

All that being said, if it doesn't blow up within the first few minutes it's probably sufficiently usable. Just don't count on some weird segfault not being the fault of the system's alleged glibc or some hilariously broken kernel module. ;-) (Kindle actually has a fairly decent kernel compared to most of these embedded devices, but to compensate for that they have an extremely untrustworthy FUSE mount.)

Also, curious, what is different about MuPDF? Koreader seems to backport a huge amount of material to mupdf-1.13.0; did something awful happen in the following release?

Yes, it changed everything about the API so that it'd be easier to maintain going forward. Porting that over constitutes an effort no one has found or made the time for.

This is slightly funny since I don't run NixOS and am frequently told that my workstation isn't a "desktop" because it omits certain software ;) But I guess all terminology is loaded in some way.

But does the concept make sense to you? ;-)

@ghost ghost marked this pull request as draft June 27, 2022 21:28
@ghost
Copy link
Author

ghost commented Jun 27, 2022

@Frenzie, thank you for patiently explaining so many things to me in this thread. You've been very generous with your time.

I've been thinking a lot about this PR. I still think that nixpkgs' amazing support for cross-compilation would make koreader's build process much simpler and more maintainable. I do a lot of cross-compilation and it's very hard to go back. However I'm no longer think it's a good idea to build the ereader/device builds of koreader from an expression kept in the nixpkgs repo. With very few exceptions, nixpkgs doesn't keep old versions of things around for very long. That policy simply doesn't work for things like koreader that have to deal with manufacturer-supplied kernels and userspaces.

I'm starting to think that the patchelf-based koreader expression is in nixpkgs not because people want to build koreader using the nixpkgs libraries, but rather because there happen to be a bunch of people who use NixOS who want to be able to conveniently use the X11 koreader to develop lua scripts for their ereaders. I'll try to be more aware of these "patchelfpkgs" expressions which are in nixpkgs mainly because NixOS doesn't have its own repo, and resist the urge to make them build from source.

We have put a lot of time and effort into https://github.com/koreader/koxtoolchain and without that

There's a similar situation with another piece of software which I am very fond of, coreboot. Curiously, coreboot's version-pinned toolchain is in nixpkgs, but coreboot itself isn't.

But does the concept make sense to you? ;-)

Yes, absolutely. Chromium and firefox both vendor enormous quantities of libraries (probably more than koreader does) but only use them if the linker can't find a corresponding system library. For situations where automatic detection is inappropriate I belive the manual option is --with-system-libfoo.

@ghost ghost closed this Jun 27, 2022
@ghost ghost deleted the pr/koreader/fromsource2 branch January 23, 2024 06:48
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants