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 issues with paths when using windows #33

Merged
merged 2 commits into from Nov 22, 2017

Conversation

Projects
None yet
2 participants
@pimterry
Member

pimterry commented Nov 22, 2017

I know we're moving away from this JS implementation soon, but it'd be very useful to get this in in the short-term anyway as a quick fix to unblock resin-io-modules/resin-device-init#25, where this currently breaks OS configuration in Windows.

Right now, reconfix breaks on windows because it tries to pass paths like \\system-connections\\resin-wifi to resin-image-fs, which is having none of it, and claims that file doesn't exist.

That is the right OS-local separator, but it completely doesn't work for resin-image-fs (as far as I can tell, that expects / all the time). This PR changes this to always generate paths like /system-connections/resin-wifi instead, which always works, regardless of the local platform.

@pimterry pimterry requested review from abrodersen and jviotti Nov 22, 2017

@pimterry pimterry changed the title from Fix issues with paths when using windows with linux images to Fix issues with paths when using windows Nov 22, 2017

@pimterry pimterry changed the title from Fix issues with paths when using windows to WIP: Fix issues with paths when using windows Nov 22, 2017

@pimterry

This comment has been minimized.

Show comment
Hide comment
@pimterry

pimterry Nov 22, 2017

Member

Ooooh, fascinating: while this fixes the resin-image-fs integration tests that use reconfix, for me on windows, it seems to break reconfix's own tests here on windows. I'll dig into that now to get appveyor passing here (marked as WIP in the meantime). This'll be fun!

I'm going to ignore the circleci error entirely, since it's not a required status, and there's no circle config on this branch, shout if that's wrong.

Member

pimterry commented Nov 22, 2017

Ooooh, fascinating: while this fixes the resin-image-fs integration tests that use reconfix, for me on windows, it seems to break reconfix's own tests here on windows. I'll dig into that now to get appveyor passing here (marked as WIP in the meantime). This'll be fun!

I'm going to ignore the circleci error entirely, since it's not a required status, and there's no circle config on this branch, shout if that's wrong.

@pimterry pimterry changed the title from WIP: Fix issues with paths when using windows to Fix issues with paths when using windows Nov 22, 2017

@pimterry

This comment has been minimized.

Show comment
Hide comment
@pimterry

pimterry Nov 22, 2017

Member

In fact, this reduced the number of tests broken (master currently fails even worse on windows), and the remaining issues were similar windows path.join bugs in the test code itself. Now fixed too, we should get everything (ignoring circle) merrily passing now 🤞

Member

pimterry commented Nov 22, 2017

In fact, this reduced the number of tests broken (master currently fails even worse on windows), and the remaining issues were similar windows path.join bugs in the test code itself. Now fixed too, we should get everything (ignoring circle) merrily passing now 🤞

@jviotti

This comment has been minimized.

Show comment
Hide comment
@jviotti

jviotti Nov 22, 2017

Member

@pimterry Great findings, I'll get you a version on npm in a minute. @abrodersen is this something we should keep an eye on on the Rust fork?

Member

jviotti commented Nov 22, 2017

@pimterry Great findings, I'll get you a version on npm in a minute. @abrodersen is this something we should keep an eye on on the Rust fork?

@jviotti jviotti merged commit cb1a72a into master Nov 22, 2017

4 of 5 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jviotti jviotti deleted the windows-fix branch Nov 22, 2017

@jviotti

This comment has been minimized.

Show comment
Hide comment
@jviotti

jviotti Nov 22, 2017

Member

@pimterry Check v0.0.4 on npm!

Member

jviotti commented Nov 22, 2017

@pimterry Check v0.0.4 on npm!

@pimterry

This comment has been minimized.

Show comment
Hide comment
@pimterry

pimterry Nov 22, 2017

Member

@jviotti perfect, thanks!

Member

pimterry commented Nov 22, 2017

@jviotti perfect, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment