joplin-desktop: build from source#417048
Conversation
d4b3a30 to
a160497
Compare
|
I reworked the plugin building to remove IFDs |
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. |
Seems like you are running joplin-desktop 2.14.17 there for some reason, not this one |
yajo
left a comment
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 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
|
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
r-vdp
left a comment
There was a problem hiding this comment.
Thanks for all your effort on this!
|
Thank you and everyone else for reviewing and helping get this over the finish line! :) |
|
@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? |
|
This seems to have introduced a runtime dependency on nodejs. Is there any way to avoid this? |
Can you elaborate what you mean by that? And is that not the case for other (source-built) Electron applications? I can look into it if you give me some pointers :) |
|
What I mean is that this package depends on the nodejs package, which has quite a large closure size. If we compare the following: with This is likely because something in the build references this, but I'd assume/hope this isn't actually needed at runtime. |
|
This is due to shebangs and other references to nodejs in the node_modules inside app.asar: I'm looking into if it's possible to eliminate those. The AppImage ships those as well, but does seem to work fine without nodejs after all. |
|
I'm not really getting anywhere, and I can't really spare more time to sink into this at the moment. Best I can tell you so far is that nodejs (and patching the shebangs that reference it) is required for building keytar during dependency installation. It looks like element-desktop builds keytar using nix and symlinks it into node_modules. I could not easily get the same to work for joplin-desktop, but maybe that way it could be avoided to pull in nodejs. |


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.