-
Notifications
You must be signed in to change notification settings - Fork 6
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
Replace pkg
with Node SEA
#68
Conversation
9628b9f
to
2685bd6
Compare
2685bd6
to
850ace2
Compare
7472f07
to
8cc5fd1
Compare
|
Hello @tuurep! This PR replaces the deprecated Anyways, I would appreciate if you could verify that this also works on your machine! You can check out this branch and then run In case you have the time I would also really appreciate if you could take a look at the code and general infrastructure. But no worries if not. Let me know if you want to look into it, then I can give you a quick run-down of the ideas and considerations I had here :) PS: I invited you as a collaborator on Vivify so if you accept you should be able to leave a "real" review on this PR😊 |
Hi, thanks I can look at this over the weekend! If you wanna give a run-down, I'm interested |
Thank you!
Okay so first of all I switched out the little build script we had for a Makefile since Node SEA is a bit more work to build at the moment:
Doing just this leaves us with all the code working, but the assets in the
Since this is very different from getting the files directly from the disk, we can't use This concludes the main gist of the changes A couple of other small details:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this works on Linux with absolutely no issues! edit: wrong
Quite a lot of setup involved with the Node SEA I see - I can't really judge it other than looks good based on your explanations as well
Now that we have a Makefile, do you think we could add Also I'm prepared to update the AUR PKGBUILD, but I have a feeling this will be an improvement for it/easier than when the About Arch Linux build dependencies, gotta note that
|
Awesome, thanks for the review!
Hm, not sure, what directory exactly would you install it to? I put everything I have to install manually into I agree that it's nice for development and testing though and while I haven't committed anything to the repo that installs Vivify because I don't know where exactly to install it to, I do have a script that does it for me. My #!/bin/sh
make macos && cp build/macos/* ~/.bin and just run this to install. Unless you have an idea for where exactly to install for everyone maybe something like this could be an option for you as well :)
Ah nice, thanks! I forgot, is there something you have to do on this repo to update it or is it entirely somewhere else? In case there is, you want to do that on this branch before we merge it or do you want to make a separate PR? I don't mind, feel free to decide :)
Oh, I didn't think about this at all! I guess |
I believe it's (at least in a general sense) standard on linux to have these types of executables in either:
Though I have heard about But it's a good question whether this works indeed for everyone, I'm thinking I could take a look at a bunch of projects' Yeah I've also been using a script for this: #!/bin/bash
cp -f ./bin/linux/viv ./bin/linux/vivify-server ~/.local/bin/ \
&& echo Installed The AUR stuff yeah it's on its own little repo and will be only relevant after the next release, so it's fine :D
Actually in the PKGBUILD there's only
I think |
I checked and on macOS the only directories that are on
So not really any option to install for a single user and (at least on macOS) all of those options would require
Nice!
Okay, I got rid of the
So I checked and while there are modules for it (tar-stream or tar), the APIs don't look nearly as nice as the one of node-stream-zip which we are currently using. Another option we could probably do is just use a script based on node-stream-zip to zip the archive and not rely on the |
Hmm. Googling around seems like https://unix.stackexchange.com/questions/8656/usr-bin-vs-usr-local-bin-on-linux
This was a pretty helpful & clear read too, about bins under I'm not sure, we could add
Yeah, I don't have an opinion whether it's bad to have |
I went through the Node SEA documentation again and realized I incorrectly copied an argument that they only list for macOS to the Linux injection as well. Can you try building it again like this? |
Nice find! Sadly it's segfaulting the same way with this change |
Okay, I hoped this would be it, haha. Maybe you could try following the example they have on the documentation and see if that works? |
Yeah! I have to get back to this tomorrow. Thanks for the big efforts today. |
Thanks to you too! Having fun working on this together! |
Hey, I didn't have time yesterday, but going to start looking at it now. If you've got any leads lmk, but I can start with the Node SEA docs for sure. btw, there is this warning in the build log:
Does that look suspicious? |
No worries! Yes, I think the docs are a good starting point. They have a full and simple example and we can see what we do next based on whether that works/how it fails :) I haven't found any new leads unfortunately, it's hard since I haven't managed to reproduce it even on (Docker) Linux. Hm, that warning does indeed seem suspicious. I checked and the CI-build also prints it. On macOS it doesn't happen, neither locally for me nor in the CI. However running the CI-build worked for me in the Linux container so I am not sure if that could actually be it |
Ah, speaking of Docker, in case we don't find any new leads based on you trying the documentation example another option would be for you to try out if you can run the build in the Docker setup that I used the other day |
Ok important lead here: I just followed the tutorial with the So the problem can be entirely separated from vivify, I guess? |
I'm not experienced using
|
Hm, interesting. I am unfortunately also not experienced with low level debugging. The only thing I get from that is that it seems it has something to do with shared libraries that are trying to be loaded and the nodejs/single-executable#46 (comment) says that
Could this be the issue? maybe your GLIBC is not compatible with whatever would be required by the Node version(s) we have tried? Other than that the only thing I could think of would be opening an issue on nodejs/single-executable. |
I looked around the shared library thing a bit more, seems like Vivify depends on these:
With Edit You can get the required shared libraries with |
Eh, I was about to test running in docker as you suggested, but found something in the meanwhile: The CI-built executables do actually work for me (as linked before: https://github.com/jannis-baum/vivify/actions/runs/9841072109) Not sure why I thought earlier they didn't. Is this with the latest commit, and how's it built compared to manual? As for the linker related stuff, when I run
But same on
Seems wildly different. What could that tell us? |
Awesome! I don't know how you tried it the first time but maybe somehow it still tried starting a version you compiled on your machine instead? E.g. if you used the
Not sure which commit exactly that was from but I think this should mean that they probably all work
I think this should mean that (for whatever reason) your All of this was just my wild speculation though ^^ Anyways, since we have found that this isn't Vivify's issue and that you can run the CI builds I would say we can merge this, right? For local development you can always use I'll just make one tiny change to the CI to get rid of a deprecation warning and then we can merge this if you don't have anything else🥳 Let me know! |
Yeah, sadly this breaks the AUR package (or rather I'll have to mark it outdated for now)
So is it possible to get the CI build for a local branch without having a PR open for it? |
Does the AUR package always have to be built on the user's machine or is there also some way to distribute compiled binaries?
If you push the branch to the repo I think the CI will run regardless of whether there is a PR. I can check in a bit, but it case it's not like that at the moment we can easily make it do that. On local branches that are not pushed to the repo however the CI of course can't run |
AFAIK in that case it should be a new AUR package with the name I could try setting that up as well.
Ok this definitely softens the blow! Well, hesitantly, I think we could merge but there's this looming unsolved problem still 😄 I'll consider making an issue at the node SEA repo. Since the SEA is at an alpha stage, perhaps things will settle with time as well. |
Nice! Then the convention is to have two packages, one for building yourself and one that is a binary?
I checked, right now it will only run on branches that have a PR to
Yes, I think opening an issue there is a good idea. Maybe it'll help make it more stable in the end, haha Edit I would suggest we merge this then (potentially after setting up the CI to run on all branches if you prefer that). I think our user base is small enough to risk using experimental software😂 So far I haven't had any problems with the binary🤞 Edit 2 I set the CI to always run :) will merge now. We can wait until we figure out what to do about the AUR package before we make the next release |
bf4cf1f
to
db54d69
Compare
Yeah, as an example for Zotero in AUR: A third variant is a But I've felt no need because of how frequently you tend to release 😄 |
Btw I have a couple of unrelated feature ideas that I'd like to ask about 😄 but it's very offtopic. Should I ask away here, or in discussions or somewhere? |
Nice! Yes, discussion sounds good :) I think this PR has enough messages now haha |
A Discussions tab isn't open on this repo, but it made sense to create an issue and add the other idea to an existing one. Edit: oh, it is now 😄 but it's okay, maybe better this way |
Close #60