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

Add node / nvm build error note to the README.md #93

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

Tweekism
Copy link
Collaborator

@Tweekism Tweekism commented Jul 15, 2024

Close #91

@Tweekism Tweekism marked this pull request as draft July 15, 2024 05:58
@Tweekism Tweekism changed the title Fix(#91): Add node / nvm build error note to the README.md Add node / nvm build error note to the README.md Jul 15, 2024
@Tweekism Tweekism marked this pull request as ready for review July 15, 2024 11:10
@Tweekism
Copy link
Collaborator Author

I've put it here for now, with the build instructions.

If you want more details it might be better on its own page, like a troubleshooting or on the contributing page. To keep the front page README.md from getting too over whelming.

@jannis-baum
Copy link
Owner

I've put it here for now, with the build instructions.

If you want more details it might be better on its own page, like a troubleshooting or on the contributing page. To keep the front page README.md from getting too over whelming.

Yes sounds good. We can just link to the contributing under "compile yourself" on the readme and then put the instructions there. Would be great if you could also figure out the minimal version of Node required for SEA and add it there as well :)

@Tweekism
Copy link
Collaborator Author

Thats a good idea, then we can include more details, like we could list minimum versions for all the dependencies if we so wished.

I'll do that now.

@Tweekism Tweekism marked this pull request as draft July 15, 2024 14:32
@jannis-baum
Copy link
Owner

Hey, just as a heads-up: I just added Markdown linting to the repo, please rebase to main and make sure your changes will pass the linting rules :)

@Tweekism Tweekism force-pushed the nvm-doc branch 4 times, most recently from 72ea75c to 8d17ebd Compare July 16, 2024 13:57
@Tweekism
Copy link
Collaborator Author

Tweekism commented Jul 22, 2024

Hi guys,

Getting back to this today...

Is jq no longer a requirement at all?

EDIT: Also is zip needed? or have you switched to tar.gz? (I see zip still referenced in the makefile)

@jannis-baum
Copy link
Owner

Awesome!

Yes, jq is no longer a dependency :) We are dependency-free at runtime now.

zip still is a build dependency. We considered switching to tar but it got a bit ugly due to differences between BSD tar (on macOS) and GNU tar (on Linux) so we ended up on the thought that it's not quite worth it, especially since it is not needed at runtime anyways

@Tweekism
Copy link
Collaborator Author

Cool.

Question for both of you @jannis-baum @tuurep, when you are doing ./configure <install_path> ...Where are you guys installing to?

I'm installing to ~/.local/bin/ because I have write access there (without needing root) and it is on $path by default, but that might be a Fedora thing, doesn't seem to be the case on Ubuntu.

@jannis-baum
Copy link
Owner

We had a discussion about that topic starting in #68 (comment) and the next few comments.

tldr; I think Tuure installs to ~/.local/bin. On macOS we don't have any user-writable directory on $PATH by default so I started my own convention a long time ago at ~/.bin which I should probably change to be the conventional ~/.local/bin ^^ Installing to something you have write permissions to seems like the most sensible approach to us :)

@jannis-baum
Copy link
Owner

Hey this is super out of context but just as a heads-up for both of you @tuurep @Tweekism: I edited your PR descriptions to explicitly say Close #<issue-number> because this makes GitHub understand that the issue is linked to the PR and will also auto-close it on merge.

Maybe this is something we should also mention in the contribution guide? @Tweekism could you add it on your PR here since you are already editing that file anyways?☺️

@Tweekism
Copy link
Collaborator Author

Sure can mate, I was wondering why I wasn't getting the little icon on the issues page, I thought I had added it.

@Tweekism Tweekism marked this pull request as ready for review July 23, 2024 13:13
@Tweekism
Copy link
Collaborator Author

Hey peeps,

This is ready for you to have a look at, sorry it took so long, since it is a lot of change I know I'm holding up anyone else who wants to update the docs. Is it too much?

Note there is one "TODO:" in there, and that is the command to install the required dependencies on MacOS. Could you let me know? Or do we not want to go to that level of detail. I kind of modeled it after the Ladybird browser project that I flirted with a little bit 😅

@tuurep
Copy link
Collaborator

tuurep commented Jul 23, 2024

Yes I install to ~/.local/bin

and it is on $path by default, but that might be a Fedora thing

Same is true false on Arch and, but probably the most general convention there is

doesn't seem to be the case on Ubuntu.

Oh.. Interesting lol

@Tweekism
Copy link
Collaborator Author

Tweekism commented Jul 23, 2024

Hey @tuurep,

Yeah I went back and had a look at that thread. Is that a default path on Arch?

edit: oh you ninja edited me, sneaky sneaky :)

@Tweekism
Copy link
Collaborator Author

doesn't seem to be the case on Ubuntu.

Oh.. Interesting lol

To be fair, I only checked on Ubuntu server, I’ll have a look on Ubuntu desktop see if it’s any different.

@tuurep
Copy link
Collaborator

tuurep commented Jul 23, 2024

Actually I went to double check if ~/.local/bin is on $PATH by default on Arch, and it's not 😄

I have this old line on my .bashrc:

# User scripts and python (pip) stuff
export PATH="$PATH:$HOME/.local/bin"

The comment reveals that I didn't know the full extent of what I was doing, but had set up ~/.local/bin initially to install stuff through pip 😄 So it hadn't been default.

Oh yeah it says user scripts too. Well, it's accurate enough.

@Tweekism
Copy link
Collaborator Author

That's interesting haha.

I'm in Windows at the moment (downloading Warzone updates 🙄). When I'm back in Linux I'll have a look at my .rc file.

Copy link
Owner

@jannis-baum jannis-baum left a comment

Choose a reason for hiding this comment

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

Hello! Thanks a lot for putting in the effort of writing such nice instructions here! I appreciate it a lot. See below for comments

@Tweekism
Copy link
Collaborator Author

Ok I've tightened it up significantly, and re-arranged a bit.

I'm going over it again double checking, let me know if thats better or if I missed anything.

Copy link
Owner

@jannis-baum jannis-baum left a comment

Choose a reason for hiding this comment

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

Nice, looks good! Let me know if everything is done from your side as well, if so I will merge this.

@Tweekism
Copy link
Collaborator Author

I just marked 2 backticks that I missed, other than that I'm happy.

@jannis-baum
Copy link
Owner

I just marked 2 backticks that I missed, other than that I'm happy.

Oh, did you fix that? I don't see any new changes.

@Tweekism
Copy link
Collaborator Author

I marked it in the code review thing but didn't commit it. Maybe you can't see them. Want me to just push a new commit?

@Tweekism
Copy link
Collaborator Author

I don't know how to do things on my phone. I have my laptop now give me 2 minutes and I'll fix up these to tiny things

@Tweekism
Copy link
Collaborator Author

Done.

I can squash those two new tiny commits into the previous one if you like.

Copy link
Owner

@jannis-baum jannis-baum left a comment

Choose a reason for hiding this comment

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

Done.

I can squash those two new tiny commits into the previous one if you like.

Yes sounds good!

@Tweekism
Copy link
Collaborator Author

Hows that look?

@jannis-baum
Copy link
Owner

Perfect! Then I will merge it, okay?👀

@jannis-baum jannis-baum merged commit 55ee01a into jannis-baum:main Jul 25, 2024
5 checks passed
@jannis-baum
Copy link
Owner

Nice! Thanks a lot again and congrats on your first official contributions to Vivify🥳

@Tweekism
Copy link
Collaborator Author

I'd like to see this project go far because you guys are so nice. I've seen so many small projects die because they were run by one dude who didn't really like to talk to people 😅

@jannis-baum
Copy link
Owner

Thanks! I am also having lots of fun here working with you guys☺️ And I also still have some big plans for Vivify ^^ Namely the Jupyter Notebook thing

@jannis-baum
Copy link
Owner

Oh, and, speaking of plans, I was thinking to set up a GitHub project board soon where the issues from this and the vivify.vim repo are added automatically and then we can order stuff by what we each want to work on next to keep things more organized. Just as a heads up, you will get an e-mail invitation or something once that's ready :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update CONTRIBUTING.md to include nvm / node install instructions
3 participants