Skip to content
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

MountableFile path resolving on Windows buggy #444

Closed
glebsts opened this issue Aug 18, 2017 · 6 comments
Closed

MountableFile path resolving on Windows buggy #444

glebsts opened this issue Aug 18, 2017 · 6 comments

Comments

@glebsts
Copy link
Contributor

glebsts commented Aug 18, 2017

Added new test into MountableFileTest.java

@Test
public void forHostFileOnWindows() throws Exception {
    final Path file = createTempFile("somepath");
    final MountableFile mountableFile = MountableFile.forHostPath(file.toString());
    /* taken from performChecks() */
    final String mountablePath = mountableFile.getResolvedPath();
    /* fails here */
    assertTrue("The resolved path can be found", new File(mountablePath).exists());
}

On Windows resolved path becomes mingw-fied as //c/something/filename.ext and File API does not understand it.
That results in files being copied with null-length i.e. when packing tar archive with files to copy to container. Header for file is created, but content is not written.

What's the idea behind mingw-fying file path?

@glebsts
Copy link
Contributor Author

glebsts commented Aug 19, 2017

PR #445 please review.

@rnorth
Copy link
Member

rnorth commented Aug 23, 2017

Thanks @glebsts, and sorry for not responding sooner. Re scope of testing on Windows, it's unfortunately not something we run automated tests on yet. As far as we're aware there are no cloud testing options that support use of docker on a windows host, so the only option seems to be physical hardware - which takes quite a bit more effort to set up. I need to give this some attention, though.

Will review.

@glebsts
Copy link
Contributor Author

glebsts commented Aug 23, 2017

Thank you @rnorth . Yes, I seem to be the only freak here with "alternative operating system", but I enjoy being a rebel. So far I have to use local patched version, as quite many changes were done :). Will make new PRs. Still not able to use docker-compose with testcontainers.

Just randomly found that in google. Not sure if free, or suits you at all, probably you've seen that.
https://stefanscherer.github.io/setup-windows-docker-ci-appveyor/

@rnorth
Copy link
Member

rnorth commented Aug 23, 2017

Thanks @glebsts. I'll have a look - I'm aware of at least two attempts to use Appveyor for testcontainers, but based on that post maybe they've changed their support for Docker!

@glebsts
Copy link
Contributor Author

glebsts commented Aug 24, 2017

@rnorth I really hope that will get some review and approval, as I would like to add another PR (about #446 ) on top of that.

@jaagupkymmel
Copy link

@rnorth Please find the time to review this, I'm waiting for this fix too.

rnorth pushed a commit that referenced this issue Sep 17, 2017
… for file source (#445)

* #444 'MountableFile path on Windows' - added separate filesystem path for file source

* #444 'MountableFile path on Windows' - made FS path only remove first slash on Windows; made recursiveTar work with file real FS paths on any platform; changed file attribute reading to work with FS path; minor renames and better exc message;

* #444 'MountableFile path on Windows' - small changelog update
@glebsts glebsts closed this as completed Sep 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants