joplin-desktop: build from source #417048
Conversation
d4b3a30 to
a160497
Compare
|
I reworked the plugin building to remove IFDs |
| src = fetchFromGitHub { | ||
| owner = "laurent22"; | ||
| repo = "joplin"; | ||
| rev = "refs/tags/v${finalAttrs.version}"; |
There was a problem hiding this comment.
I think this is simpler:
| rev = "refs/tags/v${finalAttrs.version}"; | |
| tag = "v${finalAttrs.version}"; |
There was a problem hiding this comment.
It is, but it causes an error in nix-prefetch in the update script for some reason:
fetchFromGitHub requires one of either `rev` or `tag` to be provided (not both)
As a workaround rev = null can be added, or extending the nix-prefetch flags with --rev --expr null.
I thought the way I did it would be the simplest, but if using tag is much preferred then I can of course add one of these workarounds.
There was a problem hiding this comment.
Did you make sure to add tag and remove rev? If so, nix-prefetch shouldn't fail.
Otherwise you can leave it as it is. It was just a suggestion based on code review, but if it is problematic then don't worry :)
There was a problem hiding this comment.
Yes, that's what I did. Must be a bug in nix-prefetch
|
rebased and updated to 3.3.13 |
|
I refactored buildPlugin.nix as suggested by @yajo. |
|
I tested the app but got this: Clicking restart in safe mode an launching again didn't help. I'm not sure if this is actually a failure from the new build or from some configuration with local plugins... |
Seems like you are running joplin-desktop 2.14.17 there for some reason, not this one |
There was a problem hiding this comment.
Ah! I tested the wrong PR. Now it worked!
About dialog provided this info FWIW
Joplin 3.3.13 (prod, linux)
Device: linux, 13th Gen Intel(R) Core(TM) i7-1360P
ID del Cliente: e7cf469d82bd47648320bf15c5040be8
Versión de la Sincronización: 3
Versión del Perfil: 47
Llavero Soportado: No
Alternative instance ID: -
Automatic Backlinks to note: 3.0.3
Freehand Drawing: 3.0.1
Link Graph UI: 1.5.0
Note Tabs: 1.4.0
Outline: 1.5.13
Quick Links: 1.3.2
Rich Markdown: 0.15.0
Toggle Editor Menu & Keyboard Shortcut: 1.0.1
|
rebased to resolve conflict and formatted again. successfully built and run on x86_64-linux and aarch64-darwin. |
|
I'm not very familiar with yarn packaging, but is there no way to avoid vendoring that huge lock file? |
|
Hmm, I see now that there is a comment about the vendored lockfile. How would we keep this lockfile up to date? And there is no way to do the needed changes at build time during the patch phase without vendoring the lock file? A quick search in the repo suggests that we only have a handful of vendored yarn.lock files, so most other packages seem to be able to avoid vendoring lock files. |
|
@r-vdp thank you for taking a look at this. The yarn.lock is generated and kept up to date by the update script here. My reasoning for vendoring it was:
|
|
@yuyuyureka do you have an opinion on the lock file situation here? Is there a way to handle this without vendoring? |
849290a to
6e67d7e
Compare
|
Rebased and updated to v3.4.6 (Note: that's actually a pre-release, but was updated to in #436033). This required switching to Build tested on aarch64-linux and aarch64-darwin. @r-vdp if the size of the vendored yarn.lock is an issue, we could instead store and apply the diff? That would be ~500KB, down from 1.6MB. Or we could try pruning it in the FOD and see if it breaks down the road. |
|
Yeah, the only thing that's keeping me from merging this, is indeed the size of the vendored lock file. Both of those options sound like they would improve the situation, but I don't know yarn enough to make the call on which approach would be better here. |
6e67d7e to
db9426b
Compare
|
I changed it to store only the diff 👍 |
|
fetchYarnBerryDeps should only download the tgz files and never try to compile anything or run build scripts of the dependencies. Your code mentions something about "would require unneccessary complexity to fix". Does that mean that with the original lockfile is broken with fetchYarnBerryDeps? |
|
@yuyuyureka the issue is not with fetchYarnBerryDeps, that works just fine (I see where the confusion may come from; I only meant to specify the offlineCache's size difference for a rough idea on what portion of the deps is unnecessary).
Edit: I'm taking another look at |
db9426b to
98b1de8
Compare
|
I got it working! No vendoring a modified lockfile needed anymore. I'm much happier with this. For some reason I'm getting a hash mismatch for Edit: hash mismatch is resolved |
98b1de8 to
84cecfd
Compare
|
| src = fetchFromGitHub { | ||
| owner = "laurent22"; | ||
| repo = "joplin"; | ||
| rev = "refs/tags/v${finalAttrs.version}"; |
There was a problem hiding this comment.
| rev = "refs/tags/v${finalAttrs.version}"; | |
| tag = "v${finalAttrs.version}"; |
There was a problem hiding this comment.
See #417048 (comment)
What would be preferred in your opinion?
There was a problem hiding this comment.
Ah, I didn't see that comment, sorry. The tag attribute is preferred because it avoids git getting confused when the repo contains a commit whose commit hash has the tag as a prefix, or a branch with the same name. And the refs/tags/ is also rather verbose.
So I would opt to add rev = null with a comment to explain why it is needed for now.
There was a problem hiding this comment.
rev = null in the derivation didn't work for some reason (it did back then). So I've added the workaround in the update script. Seems more fitting anyway when I think about it, since it's only relevant there.
84cecfd to
ee00dfd
Compare
|
Thank you and everyone else for reviewing and helping get this over the finish line! :) |
|
This pr makes my joplin scale 4x when NIXOS_OZONE_WL is set. |
|
@HHR2020 it scales correctly and just the same with or without NIXOS_OZONE_WL set for me (1.5x scale on sway). You should create an issue, maybe others have the same problem? |
Motivation: Upstream doesn't provide binaries for aarch64-linux.
Part of #296939
I took inspiration from the respective packages on AUR and Flathub, which are also source builds.
Unfortunately, building this package is currently broken on Darwin with
sandbox = truedue to #415328 and#416077(merged). Otherwise it does build and work.But because of this, I'd keep the bin package for now and automatically fall back to it on Darwin, to not cause any issues on existing user setups. (Any opinions on this are very welcome.)On Linux, the included "Backup" plugin doesn't load for some reason. The AUR package has the same issue and it has been reported, see JackGruber/joplin-plugin-backup#92. It works on the flatpak tho, and on this package's Darwin build as well.
Edit: fixed
More usage testing is greatly appreciated, as well as any other reviews and comments. :)
@yajo you seem to be the only active maintainer of the current package, do you want to be added as maintainer here?
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.