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

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

Merged
merged 3 commits into from
Sep 17, 2017
Merged

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

merged 3 commits into from
Sep 17, 2017

Conversation

glebsts
Copy link
Contributor

@glebsts glebsts commented Aug 19, 2017

Not sure if this is the only and correct way, so comments are more then welcome.
tar adding function checks if new File(resolvedPath).isFile().
getResolvedPath() on windows returns mingwfied path, which does not work with File API, so content of file is never copied into tar archive (but header is created, resulting in 0-bytes file inside container).

Same issue with using getResolvedPath() affects many other parts (i.e. docker-compose flows), but for now trying to solve it for copy() functionality inside dockerfile builder.

Also as I cannot run *nix tests for now, will take a look at CI results.

@glebsts
Copy link
Contributor Author

glebsts commented Aug 19, 2017

philosoraptoring on results.

… 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;
@glebsts
Copy link
Contributor Author

glebsts commented Aug 19, 2017

interesting, that mingwfied path is still critical and is used i.e. when putting DockerClientSmth.class/:ro:dummy:somethingblah... so cannot drop it at all.
commons-compress classes are smart enough to handle mixed slashes.

@glebsts
Copy link
Contributor Author

glebsts commented Aug 19, 2017

I wonder why no tests remain broken. Probably some things are not tested at all :)

@rnorth
Copy link
Member

rnorth commented Sep 16, 2017

Thanks @glebsts, and sorry for the delay in addressing this. It's passed CI on Linux and run on a Mac; just reviewing locally now to make sure I follow the logic fully.

Thank you.

@glebsts
Copy link
Contributor Author

glebsts commented Sep 16, 2017

Thank you for update, @rnorth . It worked on windows with my test consisting of 4 different containers with files copied (occasionally including node_modules folder with 31k files).
After you approve that, I will make PR for #446 . It is tiny.
Btw, any chance to somehow store tar archive with files copied for debug puprose? I kinda want to see which files really get into container. There is some file testcontainersXXXX appearing in project dir, but I was not able to open it, so mb something else.

@rnorth
Copy link
Member

rnorth commented Sep 16, 2017

Cool - out of curiosity, what environment did you run under on windows? To be honest, the possibilities (WSL, git-bash, mingw, cygwin...) make my head spin, but I'd like to start keeping track.

I'm right now having a quick play with Appveyor; it seems to have changed a lot since last time we tried, so if I can get some decent Windows CI coverage it'll, frankly, relieve some of the stress of Windows support. If it turns out to be a deep rabbit hole I won't pour too much time into it, though.

Re a debug version of the TAR, the answer is it's not possible right now. The TAR archive is constructed and streamed to the docker daemon on the fly, so never hits disk. Tee-ing the output to disk for debugging purposes (or indeed, caching) seems like a plausible improvement though. Perhaps one for a bit later unless it would really assist you right now?

@glebsts
Copy link
Contributor Author

glebsts commented Sep 16, 2017

Em... Tell me more about environment. I have cygwin and use it heavily, because it is better shell then cmd. I don't like powershell. But I run tests from Intellij, so it is not involved. Docker-machine, virtualbox based (I have many vagrants, so I had this installed and docker accepted it). Not sure if you need some env. You can ask me in slack now, if you want details.

No, right now that is not a problem, but it would save me quite a lot of time when I debugged why MountableFile does not work for me (resulting in this PR and next one). So would be nice to have it.
Caching also sounds cool - if you need to copy whole dir (I do with node apps) to build inside container, caching would significantly speed things up. Right now I can imagine preparing tar before test instead of creating new one on the fly.

@rnorth
Copy link
Member

rnorth commented Sep 17, 2017

Appveyor was a dead-end, but I've been able to at least verify this against Docker for Windows + WSL, so I'm happy to merge.
Thank you for this!

@rnorth rnorth merged commit 540f2ee into testcontainers:master Sep 17, 2017
@glebsts
Copy link
Contributor Author

glebsts commented Sep 17, 2017

Thank you @rnorth . Really sad story, because testcontainers surprisingly almost works on Windows, and it is less or more possible to create patches, but you cannot test them properly. I am 95% sure this patch won't do anything bad, but i.e. for docker-compose functionality there would be much longer PR.. when I tried to fix docker-compose (root cause is still in those MountableFile paths), I got so many red tests that decided to skip and not use docker-compose.

Did you verify Win+WSL on appveyor or smwr else? Any public links mb.

@rnorth
Copy link
Member

rnorth commented Sep 17, 2017

I've had to set up a physical machine (an ancient laptop) with Windows 10 and docker! Obviously it's not going to be run as frequently as CI would, but it's something. It's achieving coverage for DfW but not docker-machine. There doesn't seem to be an elegant way to use both on the same machine without messing about with Hyper-V mode and rebooting.

Interestingly, with Docker for Windows and WSL all the core tests (inc docker compose tests) are passing. To make it work, I had to do two things, for the record:

  • Fairly obviously, just making it talk to the docker daemon is required. I'm using TCP mode and setting DOCKER_HOST prior to running.
  • A bizarre workaround I found here that seems to regularise paths: sudo mount --bind /mnt/c /c

With this, it seems to work!

@glebsts glebsts deleted the 444-mountablefile-path branch September 13, 2021 20:51
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

Successfully merging this pull request may close these issues.

2 participants