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

Update Linux installation instructions in README.md #411

Closed
wants to merge 4 commits into from

Conversation

elch01
Copy link

@elch01 elch01 commented Jun 18, 2023

Description

Quick update of the linux install instructions and systemd service setup, please reply with feedback if needed.

@elch01 elch01 requested a review from fallenbagel as a code owner June 18, 2023 21:04
@elch01 elch01 changed the title Update README.md Update Linux installation instructions in README.md Jun 18, 2023
README.md Outdated
@@ -91,7 +104,7 @@ Environment=NODE_ENV=production
Type=exec
Restart=on-failure
WorkingDirectory=/opt/jellyseerr
ExecStart=/root/.nvm/versions/node/v18.7.0/bin/node dist/index.js
ExecStart=node dist/index.js
Copy link
Owner

Choose a reason for hiding this comment

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

node has to have an absolute path

Copy link
Author

Choose a reason for hiding this comment

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

Not required if you added the -g flag during npm install, node is then added to /bin and accessible directly. 🙂

Copy link
Owner

@fallenbagel fallenbagel Jun 19, 2023

Choose a reason for hiding this comment

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

Thats for the absolute path of node binary (best practice for systemd servicefiles). -g flag install might not be how everyone installs node. Very different ways are there.

You can add these instructions (just an example. Change wordings so it's clearer 😀 )
Something along the lines of,

  • get your node path which node
  • usually it's /usr/bin/node or in your case wherever-g installs it c:
  • assuming it is replace execstart etc

In systemd service files, it is always recommended to use absolute paths. Say the user installed node using NVM. It will not be in bin. It will be in their home directory. Meaning a systemd service cannot work with just a node dist/index.js.

Hope you understand my concerns. 😄
Just trynna make it as generic as possible so it applies to most distros. (Some distros have immutable paths so node installation using -g flag won't work)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the binary was what i meant haha sorry
That's true, i'll commit some more lines clarifying that 😄

Clarifying node path in creation of systemd service
@elch01
Copy link
Author

elch01 commented Jun 19, 2023

How does this look? :)

@fallenbagel
Copy link
Owner

How does this look? :)

Perfect. There're a few changes I wanna do to the prerequisites and the installation of node so mind if I push those to this PR?

@elch01
Copy link
Author

elch01 commented Jun 19, 2023

Perfect. There're a few changes I wanna do to the prerequisites and the installation of node so mind if I push those to this PR?

Go right ahead!

@elch01
Copy link
Author

elch01 commented Sep 7, 2023

Hello.

Any status on this PR?

Copy link
Owner

@fallenbagel fallenbagel left a comment

Choose a reason for hiding this comment

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

Please make the changes that were commented

- Git

```bash
npm install -g node yarn
Copy link
Owner

@fallenbagel fallenbagel Sep 7, 2023

Choose a reason for hiding this comment

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

Please revise this to not download node through npm. And instead add this link https://nodejs.org/en/download/package-manager and also mention alternative: https://github.com/nvm-sh/nvm

Keep yarn as the only dependency to be installed using node AFTER installing it using the above methods. Might also want to add instructions for ubuntu/debain to remove cmdlet which is also called yarn before though.

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.

2 participants