-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
hi @pkafei CI lint jobs are failing check https://github.com/ipfs/community/blob/master/CONTRIBUTING_JS.md#commits for commit guidelines and also run |
@hugomrdias I don't understand the output "Failed to extract deps-macos-10.11.0.tar.gz"- this doesn't seem like it's a JS style error. https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fjs-ipfs/detail/ipfs-cli-fun/1/pipeline |
This is Jenkins running out of disk space on the worker in question. You can tell by opening the full console output for the build and looking for log lines that ran on that worker (e.g. search the output for @hugomrdias is right though, the commit lint failed the build in an earlier step though neglected to tell anyone. Thanks Jenkins. |
@achingbrain I suspected this might be a build error. So what are the next steps? Should I ignore the error message or is there something I can do to make this test pass? |
@pkafei first fix code lint errors and commit lint errors |
@hugomrdias @achingbrain Wait, I'm confused- is the lint error based on a JS style issue or is it a build problem. If it is the latter, I'm not sure how to go about fixing the build issue. |
Just ignore this error Failed to extract deps-macos-10.11.0.tar.gz your commit looks like also in your computer run |
src/cli/utils.js
Outdated
@@ -47,6 +47,7 @@ exports.getIPFS = (argv, callback) => { | |||
// Required inline to reduce startup time | |||
const IPFS = require('../core') | |||
const node = new IPFS({ | |||
print: argv.print, |
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.
print: argv.print, | |
silent: argv.silent, |
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.
The option name is silent
, not print
.
src/core/components/libp2p.js
Outdated
@@ -96,7 +96,7 @@ module.exports = function libp2p (self) { | |||
if (err) { return callback(err) } | |||
|
|||
self._libp2pNode.peerInfo.multiaddrs.forEach((ma) => { | |||
console.log('Swarm listening on', ma.toString()) | |||
self._print('Swarm listening on ', console.log(ma.toString())) |
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.
self._print('Swarm listening on ', console.log(ma.toString())) | |
self._print('Swarm listening on', ma.toString()) |
src/core/index.js
Outdated
@@ -108,6 +108,8 @@ class IPFS extends EventEmitter { | |||
this._preload = preload(this) | |||
this._mfsPreload = mfsPreload(this) | |||
this._ipns = new IPNS(null, this) | |||
this._print = options.silent ? (() => {}) : ((msg) => console.log(msg)) |
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.
I think it might be a good idea to move printed messages into the debug log if silent=true
, so if you specify DEBUG=jsipfs* jsipfs daemon --silent
you'll still be able to access that information - what do you think?
this._print = options.silent ? (() => {}) : ((msg) => console.log(msg)) | |
this._print = this._options.silent ? this.log : console.log |
Also, we should pass the console.log
funciton directly here so that we can log multiple args.
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.
When I pass the --silent
as an option in the command line I'm still receiving Swarm addresses as output in the terminal.
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.
Try logging out the value of this._options.silent
here and argv.silent
in src/cli/utils.js
. Maybe it's not being passed in correctly?
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.
Did you figure this out in the end?
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.
Haven't had a chance to work on this in the past couple of days, but plan on getting back to this this afternoon.
Would you mind rebasing for green CI please? |
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.
Code LGTM, next steps for this PR are to add a test and document the new option on the README here.
package.json
Outdated
@@ -167,12 +167,13 @@ | |||
"read-pkg-up": "^4.0.0", | |||
"readable-stream": "3.0.6", | |||
"receptacle": "^1.3.2", | |||
"sinon": "^7.1.1", |
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.
This should be in the devDependencies
block..
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.
Looking good to me, just a couple of tweaks to the documentation and I think we'll be good for merge.
README.md
Outdated
|
||
| Type | Default | | ||
|------|---------| | ||
| string | `null` | |
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.
Type is Boolean
and default is false
README.md
Outdated
|------|---------| | ||
| string | `null` | | ||
|
||
The silent option prevents swarm info output in the cli and http client. |
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.
"Prevents all logging output from the IPFS node"
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@pkafei I took a quick look and resolved the issue with the libp2p tests. Hopefully CI will be 🍏 now. I've rebased the branch again. 🚢 🚢 🚢 |
@alanshaw That's exciting news! It won't be long |
This PR resolves #1683. This PR is not merge-ready and I wanted to highlight some of the issues I've currently ran into.
node src/cli/bin daemon --silent
does not disable printing "Swarming listening on..." and I'm not sure exactly why this option is not picked up by the core library.Also, when I use
print
as a property the peer address is printed on a new line. Not sure how to remedy this behavior but I did try usingprocess.stdout.write()
. https://stackoverflow.com/questions/6157497/node-js-printing-to-console-without-a-trailing-newlineHowever when I implemented
process.stdout.write()
the address came before the print statementprocess.stdout.write()