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

Add Dockerfile support #3

Merged
merged 6 commits into from May 3, 2023
Merged

Add Dockerfile support #3

merged 6 commits into from May 3, 2023

Conversation

eberamp
Copy link
Contributor

@eberamp eberamp commented Mar 12, 2023

Changes

  • Added Dockerfile
  • Updated README with Docker installation steps

Notes:
The usage should still be the same just inside a container, I have tested in macOS no CUDA support and works well, I just had some issues using custom profiles not sure if it's a dependency issue but will look into it later

@eberamp eberamp mentioned this pull request Mar 12, 2023
xdg-open isn't installed in this container, and isn't necessary, so just cleanly fail when that's the case.
@VoxelCubes
Copy link
Owner

VoxelCubes commented Mar 13, 2023

This is awesome, thanks for making this. It almost all worked, I did have an issue though, that being that I couldn't get it to write the output files unless I just chmodded the directory to allow everyone to read and write, which isn't super ideal.
Any idea how to fix that?

I also looked into the thing with profiles. Naturally docker can't see the ones you have natively, so you'd need to add one yourself with pcleaner profile new <some name>. There are no tools to edit them inside the container, so I added vim and nano as options to be able to edit them at all. Let me know if there is a better way.

I did notice that pcleaner would assume that there will be xdg-open present to open the new profile after creating it. Currently it just fails (you can ignore it, the profile is created either way), but I included a patch to check if xdg-open exists first.

PS. docker complained that the image build system was deprecated and will be removed soon, but substituting it with the new buildx worked just fine. Does it make sense to include deprecated instructions in the readme?

@eberamp
Copy link
Contributor Author

eberamp commented Mar 14, 2023

I couldn't get it to write the output files unless I just chmodded the directory to allow everyone to read and write

What OS are you using? Are you using WSL(windows) by any chance?

Naturally docker can't see the ones you have natively

This should be possible attaching the volume though not sure exactly where they're stored haven't taken a good look at the code, the problem I encountered was when creating a new profile, I think it was a dependency issue will try to look exactly what was it this week.

There are no tools to edit them inside the container, so I added vim and nano as options to be able to edit them at all

Yeah the base image does not include those tools, I don't think they would be necessary, also you can edit them outside the container and changes should be reflected in the file since you've got the directory attached as volume, but don't use vim to edit outside because it actually doesn't write out to the file directly

docker complained that the image build system was deprecated and will be removed soon

You mean the command build image? What version of Docker are you using, I'm currently using 20.10.23

@VoxelCubes
Copy link
Owner

VoxelCubes commented Mar 15, 2023

What OS are you using? Are you using WSL(windows) by any chance?

I'm on Linux exclusively. I can get it to work by allowing write access for all users on my folder, but it would be nicer if there were a way to let the container act as my user. I tried doing that, but just ended up with a nameless container for some reason (I don't know what I'm doing when it comes to docker). But if it can be fixed by adding the docker user to some group I guess that works too. Might just be a me-problem that an experienced docker user would know how to solve.

the problem I encountered was when creating a new profile

That was an error caused by me assuming there would always be xdg-open installed on a Linux system, not thinking about minimalist installs like docker images that don't have that. It's a utility for opening a file with the system default application. It failing wasn't an issue though, and I suppressed the error message with commit 3664bde.

Oh yeah, on Linux profiles go to XDG_CONFIG_HOME/pcleaner by default, which is typically ~/.config/pcleaner. (Also works when xdg isn't configured on the host, like in this container). You can of course give a different path for a profile.

I gave it a test and loading the profile worked just fine.

you can edit them outside the container

That is, if you can even find where the hecc docker-internal files are stored. If it were saved inside the mounted volume, that'd be easy to find, I guess, but that isn't the default location pcleaner uses. Rather, it has some pcleaner user account inside the container somewhere. Accessing those with the text editors inside the container worked like a charm though, so I guess it's useful as a backup. Good to know about that re-link on write quirk, that could cause some nasty headaches!

docker version

I have version 23.0.1 (~1 month old), Buildx was introduced in version 19.03. (~2 years old). Your version came out approx. 2 months ago, so relatively new as well.
It just changes docker image build -t pcleaner:v1 . to docker buildx build -t pcleaner:v1 .
so no big deal.

Thanks for looking into it, hope this clears a few things up.

@eberamp
Copy link
Contributor Author

eberamp commented Mar 16, 2023

Well I'm not really that experienced managing containers lol but-

I can get it to work by allowing write access for all users on my folder

I don't really understand the problem, I mean I understand but I guess what I'm asking is why it's happening to you, I didn't have problems, which doesn't mean nobody will have them, but exactly what output files you mean, the cleaned images?

The user that is created within the container should have access to the bind mount and be able to write to it, but then again I'm using macOS, not sure if it's specific to linux

but it would be nicer if there were a way to let the container act as my user.

There is different ways to do it but should not be necessary

on Linux profiles go to XDG_CONFIG_HOME/pcleaner by default, which is typically ~/.config/pcleaner.
...
That is, if you can even find where the hecc docker-internal files are stored.

Right, now I see the predicament, no yeah it'd be better if we install nano inside the container, I thought you meant like existing files you already have in your host or new files that were written to the current directory (mounted directory)

Let's explore the issue with Linux if you can give me more context, I'll try to run a Linux VM and try to replicate

@VoxelCubes
Copy link
Owner

VoxelCubes commented Mar 16, 2023

Yeah, it fails to write the cleaned images to the mounted volume (permission denied), since it only has read access by default, and the container acts as a different user, not my current user that owns the files. Changing that 1001 to 1000 actually completely fixed the issue, it now works as before, just minus that problem. Was there a reason to have it not be 1000?

This article seems to explain it better than I could, the article you linked also helped me here.

Not sure why the issue wasn't on MacOS, maybe something to do with how it's virtualized(?), but do let me know if the following patch breaks that platform. If it doesn't, I think the dockerfile is good to go. :)

PS. I also added a few changes to the readme, let me know if you can improve it further or I made a mistake.

When using UID 1000, the container will act as the current user (provided it is also UID 1000) thereby not requiring any special access modifiers to write files to the mounted volume.
@eberamp
Copy link
Contributor Author

eberamp commented Mar 24, 2023

maybe something to do with how it's virtualized

You may be right!, actually the way docker works in macOS is via a lightweight VM contrary to Linux where containers run in a native way 🤔

Digging a little deeper I found that

In macOS, the osxfs driver pretends that the files are owned by the USER that the container runs as. If you are seeing mounted files as being owned by root, your container probably is set to run as root.

There's no harm done in changing the user id to match the same user id from the host, we can either pass it through the $UID variable but probably there's a better way to do it, let me investigate a little and I'll update the Dockerfile

@VoxelCubes
Copy link
Owner

Ah yeah, that does explain why there was no problem on macos, mystery solved.

All right, once you're happy with the results and finished tweaking it, I'll merge.
Thanks a lot, I learned a bit about docker :)

Repository owner deleted a comment from Fujicrow Apr 9, 2023
Repository owner deleted a comment from Fujicrow Apr 9, 2023
@VoxelCubes
Copy link
Owner

Hey, have you come up with any changes? If not, I suppose it's fit for merging next week. Thanks for all you've done!

@VoxelCubes VoxelCubes merged commit 91e8440 into VoxelCubes:master May 3, 2023
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.

None yet

2 participants