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.

Sign up for GitHub

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

Jump to bottom

Fix npm install failed #67

Closed
wants to merge 5 commits into from
Closed

Fix npm install failed #67

wants to merge 5 commits into from

Conversation

daxiondi
Copy link
Contributor

This error occurs with npm install

[email protected] postinstall /Users/cgland/Documents/jitsi/handbook/website/node_modules/gifsicle
> node lib/install.js

  ⚠ connect ECONNREFUSED 0.0.0.0:443
  ⚠ gifsicle pre-build test failed
  ℹ compiling from source
  ✖ Error: Command failed: /bin/sh -c autoreconf -ivf
/bin/sh: autoreconf: command not found


    at /Users/cgland/Documents/jitsi/handbook/website/node_modules/execa/index.js:231:11
    at processTicksAndRejections (internal/process/task_queues.js:85:5)
npm WARN website No repository field.
npm WARN website No license field.

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] postinstall: `node lib/install.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] postinstall script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/cgland/.npm/_logs/2020-07-11T03_04_31_633Z-debug.log

I saw a lot of the same mistakes on the Internet. Please see issue

I solved this problem locally and submitted PR, please review to avoid more people having the same problem

@daxiondi
Copy link
Contributor Author

@saghul Hi, I'm here to contribute to jitsi, please review

toby63
toby63 suggested changes Jul 11, 2020
View reviewed changes
README.md Outdated
@@ -8,6 +8,30 @@ The site is built automatically with every push thanks to a [GH Actions](https:/

If you want to build it locally, follow these simple steps:

If you use windows, please execute
Copy link
Contributor

@toby63 toby63 Jul 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I suggest that what you added should be in a seperate section, maybe called dependencies or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. I have finished the revision as you said. Do you have any comments

README.md Outdated
```js
npm install -g gifsicle
```
perhaps
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you mean alternative command with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

README.md
If you use Linux, please execute

```shell
apt-get update

This comment was marked as outdated.

Sign in to view

README.md Outdated
```shell
brew install automake
```

```js
Copy link
Contributor

@toby63 toby63 Jul 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Everything after line 34 should then be seperated as it applies to all Operating Systems (as it seems).
So after the section dependencies, the section building the site should follow.

@toby63
Copy link
Contributor

toby63 commented Jul 11, 2020 •
edited
Loading

Thx for the changes so far.
Some more thoughts:
Instead of please execute you might say what is done, like install gifsicle.
Also:

  • for coherent syntax you might add a : after it.
  • maybe write the Operating Systems in bold text, like: Windows, so it can be distinguished easier.
  • and use the code-tag for the software names, like: gifsicle

Example:
If you use Windows you need install gifsicle:

Edit: A rebase after changes are complete (at best wait for @saghul to review it first) would also be good 🙂.
Thank you for your patience.

@daxiondi
Copy link
Contributor Author

Thank you for your review. About 10 minutes, I still have a PR in this warehouse. Can you help me with the review

@toby63
Copy link
Contributor

toby63 commented Jul 11, 2020 •
edited
Loading

Ok two things:

  1. Please seperate your commit f669ce3 into a new pull request.

  2. The changes in c629049 are not entirely correct.
    For Debian and Mac you install different packages, not gifsicle.

I am now afk, but I (or someone else) will look at your PRs in some time.
Thank you for contributing so far 👍.

@daxiondi
Copy link
Contributor Author

Thank you for your review. I did as you said. Good night

toby63
toby63 suggested changes Jul 11, 2020
View reviewed changes
README.md
@@ -14,14 +14,14 @@ alternative command
cnpm install gifsicle
```

If you use **Debian/Ubuntu**, you need install ```gifsicle```:
If you use **Debian/Ubuntu**, you need install ```dh-autoreconf```:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just write install.
Otherwise it should be: you need to install
The same applies below for Mac.

toby63
toby63 suggested changes Jul 11, 2020
View reviewed changes
README.md
```js
npm install -g gifsicle
```
alternative command
Copy link
Contributor

Choose a reason for hiding this comment

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

Add : behind alternative command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add : behind alternative command.

I don't know why my submission of this PR is not tracking, I re submitted a PR, please review

@daxiondi daxiondi closed this Jul 13, 2020
@daxiondi daxiondi mentioned this pull request Jul 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toby63 toby63 toby63 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@daxiondi @toby63