-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
Fix appveyor #2137
Fix appveyor #2137
Conversation
efaa139
to
d6a1668
Compare
No idea why circle ci is unhappy :( |
It is happy for me 😄 |
@chriscool could we get some advice here? the sharness tests fail on windows CI because of path separator issues (at least, thats what it seems here). What would be a good way to do this? |
As far as I can see the first error is this:
As permissions on Windows are working differently than on Linux/MacOS. It could be expected that the error message would not be the same on Windows. Maybe this could be tested on Windows and either the error message we test against could be changed when testing on Windows or if the test is not relevant it could be skipped on Windows (maybe with a UNIXPERM prerequisite or something). I don't have access to a Windows machine here. I will try to have a look at other test that are failing though. |
I think we should merge this in for now. It still doesnt pass appveyor tests, but its gets a little farther and we can move forward from there |
👍 for merging. At least actual tests are now failing |
There were the following issues with your Pull Request
We ask for a few features in the commit message for Open Source licensing hygiene and commit message clarity. This message was auto-generated by https://gitcop.com |
@whyrusleeping just rebased and squashed. Can we get this merged now? |
@dignifiedquire is it actually fixed? it says:
|
It was :( but it looks like something went wrong/changed after the rebase. |
License: MIT Signed-off-by: rht <[email protected]>
License: MIT Signed-off-by: dignifiedquire <[email protected]>
@jbenet that was close, luckily I had the fixed version on my other machine. Should be better now |
@@ -23,6 +23,8 @@ import ( | |||
manet "github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-multiaddr-net" | |||
) | |||
|
|||
var setupOpt = func(cmd *exec.Cmd) {} |
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.
were these changes contributed to iptb
proper?
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.
Yes, this was just pulled in from iptb
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.
ah ok!
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.
ipfs/iptb@b21f95b is the commit I think
Ok great! this LGTM. down to merge it before fixing the bugs. But before-- the iptb stuff should be updated in the iptb repo (https://github.com/whyrusleeping/iptb) and then update the dep here. |
Thanks @dignifiedquire, @whyrusleeping, @chriscool, @Kubuxu, and whoever else was involved -- it's a big deal to get this working :) |
You have mostly to thank 32C3 dark room hack sessions :D |
No description provided.