ipfs/js-ipfsd-ctl

Make testing fast again - Bring init options back

Closed this issue · 14 comments

richardschneider commented

Spawning a deamon on my machine (windows) is extremely slow. It appears that each go/js command that is run takes ~10 seconds!

More details to follow.

dryajov commented

10s is definitely a little slow, but keep in mind that each spawned daemon (unless pointing to an existing repo) will have to init the repo and apply any custom configs before launching the damon, which could get pretty slow. But please do provide details, I'd like to speed this up as much as possible 👍

richardschneider commented

Its not just ipfs init. Been sided tracked with other issues, but should have details in an hour or so.

FWI: Timing was done by hand (one onesecond, two one-second, ...) and each command (init, config) took exactly 10s.

Also, are the config settings applied to in-proc IPFS?

daviddias commented

@dryajov when the new version of ipfsd-ctl, we lost the ability to set key size on init. This is really unfortunate as tests are taking forever nowadays.

My proposed solution:
image

init should be a bool or object and the object should accept any of the init options:

image

daviddias commented

@richardschneider we are not going to patch globals. This should be an option and not a surprise.

dryajov commented

@dryajov when the new version of ipfsd-ctl, we lost the ability to set key size on init. This is really unfortunate as tests are taking forever nowadays.

We didn't, you can still set it as part of calling ipfsd.init({keysize: <size>}). The init flag was always a bool -
https://github.com/ipfs/js-ipfsd-ctl/blob/v0.26.0/src/index.js#L9..L15.

That said, I agree it's a valuable option to have. In order to not break the existent implementation, lets leave the init as a bool, but add a new property, initOpts that would take a keysize, that would also support any future init option that we might require.

daviddias commented

I'm curious why the update of js-ipfs, js-ipfs-api and ipfs/interop didn't contemplate that ipfsd.init with reduced key sizes. Was there any blocker?

dryajov commented

I don't believe we ever ran our tests with reduced key sizes by default? That said I think we can have our cake and eat it too. Here is what I propose:

  • add the ability to pass the keysize to spawn (could be by modifying the existing init to take an object or adding an initOpts property, whatever works best) (thanks @diasdavid )
  • add the ability to pass the keysize with env variables in addition to passing to spawn, we need one for each - js, go and proc
  • modify package.json to run test with the desired keysize by passing it through ENV variables

Does this sound as an acceptable solution?

Updated:

  • Added clarification
dryajov commented

we need one for each - js, go and proc

I'm actually not sure if we need this. In @richardschneider PR #198, there is a check that uses 1024 for go and 512 for js, I'm confused as to why this is, but if we don't need to use different sizes, then we don't need separate variables.

richardschneider commented

@dryajov go-ipfs enforces min key size of 1024, whereas js-ipfs is happy with 512.

@diasdavid

we are not going to patch globals.

It is not patching a global. It is just using global to detect if mocha is being used.

This should be an option and not a surprise.

Just document that the default key size is 2048 unless running in a mocha test. In which case it's 512 for js and 1024 for go.

dryajov commented

@diasdavid @richardschneider added #203 implementing what I outlined in #188 (comment)

daviddias commented

I don't believe we ever ran our tests with reduced key sizes by default?

It was there until the new ipfsd-ctl appeared. A quick git history search:

add the ability to pass the keysize to spawn (could be by modifying the existing init to take an object or adding an initOpts property, whatever works best) (thanks @diasdavid )

Let's do this one only and not reinvent the wheel. ipfs init has args, let's use them:

image

The propname should be bits.

Let's follow the proposal on here: #188 (comment) Let me know if you have questions.

  • 👍1
dryajov commented

👍 on reusing ini options

daviddias commented

let's finish this one on #203