os agnostic

[richardschneider]

The key path is always OS agnostic and is separated with a /. This allows callers to use the standard key name without having to worry about the runtime OS.

These changes are an improvement on #11 and get all tests to work.

IMHO: #5 is wrong.

Hopefully this will help js-ipfs move forwards on a window platform (ipfs/js-ipfs#1017, ipfs/js-ipfs-repo#144 and ipfs/js-datastore-core#2)

[daviddias]

@richardschneider could you confirm that this feature does not break go-ipfs interop? @dryajov might be able to help :)

[dryajov]

I think this PR makes a lot of sense, what I'm not sure about is why the initial os specific change was introduced and I haven't had the time to dig into the code to figure it out. Perhaps, @dignifiedquire has some insights as to why the original change was introduced?

@diasdavid I can make sure that interop with Go doesn't break :)

[richardschneider]

I'm pretty sure this does not break any go-ipfs interop, I'm using the new code with js-datastore-fs which has a test for interop with go and it passes.

[Kubuxu]

EDIT: I read this PR wrong. It removes os specific component from the interface-datastore.

---

The repo should also have interop from Windows to Linux and back. So please check if keys used in for example leveldb are the same on both those systems. For me it seems that platform specific stuff should be done in flatfs and other datastore implementations that touch file system instead of here but I might be wrong.

[richardschneider]

@Kubuxu Is this comment directed at me? If so I'm not sure what ur asking.

[dryajov]

@Kubuxu This is part of making js-ipfs run on windows. So far I don't think we have any testing going on windows, but we'll keep this in mind - thanks!

@richardschneider I believe, this might potentially require changes to each implementation to account for path conversions for each os? I'll spend some time today looking at the code as well to have a better idea of what else is required.

[Kubuxu]

What I am saying is:
datastore can be backed by multiple types of repos: leveldb, badgerds (in Go) and flatfs (and many more). Keys should be exactly the same in all of those repos on all architectures and flatfs (as it currently is the only repo that touches file system so its keys are affected by different separator) should do key translation.

[Kubuxu]

The usage of path.sep in key "class" might had been invalid "try" for Windows compatibility that isn't needed in this place from the begging.

@whyrusleeping could you take a look at this

I didn't write this code and I don't understand fully how js's ipfs stack works but something here smells fishy.

[richardschneider]

@dryajov The PR is all about making key paths os-agnostic, always using '/'. I've applied it to js-datastore-core and js-datastore-fs and they now work on windows.

[Kubuxu]

@richardschneider aghr, I always looked at this PR late into the night. You are right, you are removing the usage of path.sep here.

[dryajov]

@Kubuxu that makes sense. In that case I have a couple of questions:

• Are keys os specific in go?
  • if so, what is the reasoning behind that?

My current understanding of datastore keys is that they should be OS agnostic, and the underlying implementation (fs, leveldb, badger, etc) would convert the hierarchical key to a representation that is specific to its underlying store, i.e. datastore-fs, should take care of making the key separators into some sort of fs structure. (Take this with a grain of salt, I have not spent much time looking into either the implementations nor the spec, so my reasoning might be a bit of...)

[Kubuxu]

@richardschneider please accept my apology for the mess I caused in this PR. I didn't notice that it was removing the path.sep usage here and making things as they should be.

[dryajov]

uppps... accidentally closed the PR, ignore.

[Kubuxu]

@dryajov keys have the same separator on all platforms in Go /. The flatfs (sharding datastore?) uses system specific function to join paths but it also never receives key with multiple path elements.

What it receives currently is /CIQEMFC2HN3HPCUI6ZM2BVVUQ6VXLWNJVQZSCW3E6BNL2GT25PMS22I where the long string is base32 encoded bytes of CID (with no multibase prefix).

[Kubuxu]

datastore-fs, should take care of making the key separators into some sort of fs structure.

Yes. This PR is correct in this regard.

Side note: in last few days I have misinterpreted quite few PRs for some reason.

[dryajov]

@Kubuxu no worries at all, thanks for jumping in 👍

[richardschneider]

why is appveyor build not working? Seems to be some infrastructure issue. It was working a few days ago, https://ci.appveyor.com/project/dignifiedquire/interface-datastore/build/27

[dignifiedquire]

no need to do this, will be picked up from the commits on release

[dignifiedquire]

The code looks good to me, but before we merge, lets make sure to have a windows specific version of https://github.com/ipfs/js-ipfs-repo/blob/master/test/interop-test.js using this code to make sure things work as expected

[richardschneider]

@dignifiedquire okay will fork and test

[richardschneider]

@dignifiedquire tried running tests for js-ipfs-repo it fails with an error in levelup

 1) IPFS Repo Tests on on Node.js default "before all" hook:
: The filename, directory name, or volume label syntax is incorrect.s-ipfs-repo\test\test-repo-for-1509616898591\datastore/MANIFEST-000052

      at c:\Users\Owner\Documents\GitHub\js-ipfs-repo\node_modules\levelup\lib\levelup.js:117:34
      at c:\Users\Owner\Documents\GitHub\js-ipfs-repo\node_modules\leveldown\node_modules\abstract-leveldown\abstract-leveldown.js:39:16

Looks like levelup needs some TLC for windows.

[dignifiedquire]

looks like the path is not fully normalized and mixes slash types

…

On 2. Nov 2017, 11:17 +0100, Richard Schneider ***@***.***>, wrote: @dignifiedquire tried running tests for js-ipfs-repo it fails with an error in levelup 1) IPFS Repo Tests on on Node.js default "before all" hook: : The filename, directory name, or volume label syntax is incorrect.s-ipfs-repo\test\test-repo-for-1509616898591\datastore/MANIFEST-000052 at c:\Users\Owner\Documents\GitHub\js-ipfs-repo\node_modules\levelup\lib\levelup.js:117:34 at c:\Users\Owner\Documents\GitHub\js-ipfs-repo\node_modules\leveldown\node_modules\abstract-leveldown\abstract-leveldown.js:39:16 Looks like levelup needs some TLC for windows. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[richardschneider]

I got this error when running from the source in the repo. My interface-datastore code is NOT is not being used. See ipfs/js-ipfs-repo#144

[dryajov]

@dignifiedquire I believe that this path normalization should happen at the level of the specific datastore implementation, i.e. https://github.com/ipfs/js-datastore-core/blob/master/src/shard.js in this case?

[richardschneider]

@dryajov I tested js-datastore-core with this PR and every test passed.

However, the interface-datastore.tests against a sharding store were disabled because not all of the query options where implemented in the sharding store. I've corrected this in PR ipfs/js-datastore-core#3

[richardschneider]

@dryajov @diasdavid @dignifiedquire is there anyway you guys can progress PR or ask for changes. I have a number of the PRs for windows that are waiting on this. Ideally a new patch release to NPM would be great.

[dignifiedquire]

I will see that I give them a round of review tomorrow

…

On 3. Nov 2017, 02:05 +0100, Richard Schneider ***@***.***>, wrote: @dryajov @diasdavid @dignifiedquire is there anyway you guys can progress PR or ask for changes. I have a number of the PRs for windows that are waiting on this. Ideally a new patch release to NPM would be great. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[dryajov]

@richardschneider let me do a quick review and see if @diasdavid or @dignifiedquire can do a release.

[dryajov]

LGTM

[richardschneider]

@diasdavid or @dignifiedquire anyone want to release?

[whyrusleeping]

They are both asleep for a few more hours (european time)

…

On Nov 2, 2017, 11:06 PM -0500, Richard Schneider ***@***.***>, wrote: @diasdavid or @dignifiedquire anyone want to release? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[daviddias]

@richardschneider thank you for being awesome and pushing the Windows support throughout multiple modules!

I'm going to start reviewing all of this and to make it easier in all of us, I'm going to merge this PR into a direct branch of this repo. I've invited you to the JavaScript Team of IPFS so that you can contribute to the branch too, Welcome to the party 🎉