Windows interop #195

richardschneider · 2017-11-08T14:21:01Z

No description provided.

It is still failing, because it uses IPFS for testing and IPFS is not yet windows ready.

richardschneider · 2017-11-08T14:27:35Z

The with-dag-api tests are still failing, because it uses IPFS for testing and IPFS is not yet windows ready. Issue #196

victorb · 2017-11-08T19:39:54Z

test/with-dag-api.js

          start: false
        })

-        node.on('ready', done)


Does this not already log the error if there is one?

Sorry, it was just debug code. I'll revert to original.

victorb · 2017-11-08T19:41:00Z

src/exporter/object.js

@@ -10,7 +9,7 @@ module.exports = (node, name, pathRest, ipldResolver, resolve) => {
  if (pathRest.length) {
    const pathElem = pathRest.shift()
    newNode = node[pathElem]
-    const newName = path.join(name, pathElem)
+    const newName = name + '/' + pathElem


Seems slightly correct but not 100%. What if name ends with a slash or pathElem starts with a slash? In that case, we'll have double-slash, while path.join would handle it correctly (but not correctly on windows)

Totally agree with you. Ideally I should be using be path.posix.join which works just fine on Node. However, in the Browser it fails with TypeError: Cannot read property 'join' of undefined] . Must be a babel/webpack issue.

Let's make sure to not let this one pass. Please add a test for @victorbjelkholm's case and ensure it works both in browser and node.js

The test case (guard) you want can never happen. name is really a hash, so it can not have shash. pathElem comes from pathRest which is an array that is path separated.

# Conflicts: # test/hamt.js

richardschneider · 2017-11-09T00:52:53Z

@diasdavid I've spent the day trying to get the tests to work. But the tests, especially in the Browser, just keep failing non-deterministically.

Playing with test timeouts is non-productive. My gut feel is that some evil exists between aysnc and pull. I see that others are looking into this.

There are a few os-specific issues that I've addressed. Could we just get them into master.

daviddias · 2017-11-09T10:49:33Z

@pgte can we have you reviewing this one?

pgte

Generally speaking, given that this package does not touch the material host file-system, and that it's named unixfs, and that UNIX exposes a file system where paths are separated by the / character, I think that the path separators should not be changed.

I think that it should be the responsibility of package that accesses the filesystem (js-ipfs) to translate these paths into native FS paths. Wouldn't you agree?

richardschneider · 2017-11-09T11:37:29Z

@pgte I agree.

This package's purpose is to return a path in POSIX format (forward slashes). It fails when running on Windows, by returning back slashes. This is because it uses path.join which is os specific.

The essential change is here

pgte · 2017-11-09T11:40:38Z

@richardschneider OK, sorry, misunderstood your intent, it's clear now for me, Thanks!

pgte

LGTM! 👍

richardschneider · 2017-11-09T11:50:44Z

@pgte Not sure of the protocol. Do I merge the PR now, and wait for @diasdavid to then release it NPM?

pgte · 2017-11-09T11:52:21Z

not sure, usually it's @diasdavid that merges..

daviddias · 2017-11-09T12:14:14Z

package.json

@@ -11,7 +11,7 @@
    "build": "aegir build",
    "test": "aegir test",
    "test:node": "aegir test --target node",
-    "test:browser": "aegir test --target browser",
+    "test:browser": "aegir test --target browser --timeout 30000",


Please do not use global timeouts. We want to have timeouts per test.

daviddias · 2017-11-09T12:15:05Z

src/exporter/object.js

@@ -10,7 +9,7 @@ module.exports = (node, name, pathRest, ipldResolver, resolve) => {
  if (pathRest.length) {
    const pathElem = pathRest.shift()
    newNode = node[pathElem]
-    const newName = path.join(name, pathElem)
+    const newName = name + '/' + pathElem


Let's make sure to not let this one pass. Please add a test for @victorbjelkholm's case and ensure it works both in browser and node.js

daviddias · 2017-11-09T12:15:36Z

test/browser.js

-  // require('./exporter')(repo)
-  // require('./exporter-subtree')(repo)
+  require('./exporter')(repo)
+  require('./exporter-subtree')(repo)


Why enable these?

daviddias · 2017-11-09T12:16:10Z

test/exporter-subtree.js

@@ -36,7 +36,7 @@ module.exports = (repo) => {
          fileEql(files[0], smallFile, done)
        })
      )
-    })
+    }).timeout(10 * 1000)


This is a small nitpick but a useful one. We prefer to have the timeouts at the top of the test so that it is easy to find it.

daviddias · 2017-11-09T12:17:01Z

test/with-dag-api.js

@@ -105,7 +107,8 @@ const strategyOverrides = {

 }

-describe('with dag-api', () => {
+// TODO: waiting for IPFS support on windows, https://github.com/ipfs/js-ipfs-unixfs-engine/issues/196
+describe.skip('with dag-api', () => {


Please do not skip tests. I understand that you need js-ipfs support on windows but enabling this test will only make Appveyor fail, which is fine while Windows Support is a work in progress

daviddias · 2017-11-09T12:18:10Z

All CI is red, something must have gone wrong.

richardschneider · 2017-11-09T23:33:58Z

As usual, the Browser tests are failing.

This reverts commit c6f2bff.

…[email protected]

richardschneider · 2017-11-10T01:59:59Z

@diasdavid LGTM could you please review again

richardschneider added 2 commits November 9, 2017 03:16

fix: export files with a POSIX path

77b915e

test(with-dag-api): some fixup for windows

880ae4d

It is still failing, because it uses IPFS for testing and IPFS is not yet windows ready.

ghost assigned richardschneider Nov 8, 2017

ghost added the status/in-progress In progress label Nov 8, 2017

chore: run on appveyor

2773541

victorb reviewed Nov 8, 2017

View reviewed changes

richardschneider added 8 commits November 9, 2017 11:19

test: run more tests in the browser

c6f2bff

test: timeout

fc58627

test: remove debug code

06796a9

test: fix lint issues

229f4b1

Merge branch 'master' into windows-interop

74a2c8a

# Conflicts: # test/hamt.js

test(browser): are taking forever!!!

1a942d9

test: skip with-dag-api until #196 is resolved

b4bba29

test: more timeouts

1e8aaad

richardschneider requested a review from daviddias November 9, 2017 00:45

daviddias requested a review from pgte November 9, 2017 10:49

pgte suggested changes Nov 9, 2017

View reviewed changes

richardschneider closed this Nov 9, 2017

ghost removed the status/in-progress In progress label Nov 9, 2017

pgte reopened this Nov 9, 2017

ghost assigned pgte Nov 9, 2017

ghost added the status/in-progress In progress label Nov 9, 2017

pgte removed their assignment Nov 9, 2017

pgte previously approved these changes Nov 9, 2017

View reviewed changes

daviddias suggested changes Nov 9, 2017

View reviewed changes

Revert "test: run more tests in the browser"

481453d

This reverts commit c6f2bff.

richardschneider dismissed pgte’s stale review via 481453d November 9, 2017 23:40

richardschneider added 5 commits November 10, 2017 13:11

test: more testing issues

227cfff

chore: npm ERR! peer dep missing: ajv@^5.0.0, required by ajv-keyword…

097472c

…[email protected]

test: lint issues

7e59d26

test: more issues

badb634

test: follow the standards

c2842e6

richardschneider and others added 2 commits November 10, 2017 15:12

test: [email protected] breaks our tests

729177f

chore: bump deps

377696d

ghost assigned daviddias Nov 10, 2017

daviddias previously approved these changes Nov 10, 2017

View reviewed changes

chore: bump timeouts for nodejs 6

6274a6c

daviddias dismissed their stale review via 6274a6c November 10, 2017 10:19

daviddias approved these changes Nov 10, 2017

View reviewed changes

daviddias merged commit aa21ff3 into master Nov 10, 2017

daviddias deleted the windows-interop branch November 10, 2017 10:40

ghost removed the status/in-progress In progress label Nov 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows interop #195

Windows interop #195

richardschneider commented Nov 8, 2017

richardschneider commented Nov 8, 2017 •

edited

Loading

victorb Nov 8, 2017

richardschneider Nov 8, 2017

victorb Nov 8, 2017

richardschneider Nov 8, 2017

daviddias Nov 9, 2017

richardschneider Nov 10, 2017

richardschneider commented Nov 9, 2017

daviddias commented Nov 9, 2017 •

edited

Loading

pgte left a comment

richardschneider commented Nov 9, 2017 •

edited

Loading

pgte commented Nov 9, 2017

pgte left a comment

richardschneider commented Nov 9, 2017

pgte commented Nov 9, 2017

daviddias Nov 9, 2017

daviddias Nov 9, 2017

daviddias Nov 9, 2017

daviddias Nov 9, 2017

daviddias Nov 9, 2017

daviddias commented Nov 9, 2017

richardschneider commented Nov 9, 2017

richardschneider commented Nov 10, 2017

Windows interop #195

Windows interop #195

Conversation

richardschneider commented Nov 8, 2017

richardschneider commented Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardschneider commented Nov 9, 2017

daviddias commented Nov 9, 2017 • edited Loading

pgte left a comment

Choose a reason for hiding this comment

richardschneider commented Nov 9, 2017 • edited Loading

pgte commented Nov 9, 2017

pgte left a comment

Choose a reason for hiding this comment

richardschneider commented Nov 9, 2017

pgte commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Nov 9, 2017

richardschneider commented Nov 9, 2017

richardschneider commented Nov 10, 2017

richardschneider commented Nov 8, 2017 •

edited

Loading

daviddias commented Nov 9, 2017 •

edited

Loading

richardschneider commented Nov 9, 2017 •

edited

Loading