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

Fix local docker build of PDF spec #579

Merged

Conversation

gmlueck
Copy link
Contributor

@gmlueck gmlueck commented Jul 2, 2024

Something changed in the latest docker image that causes asciidoctor-pdf to fail when run locally (but not when run in CI). The problem is that the user is different when running locally, and this user does not have write access to the HOME directory. It appears that asciidoctor-pdf attempts to write temporary files into HOME, so this failed with an access permission error.

Fix this by setting HOME=/tmp when doing a local docker build.

Something changed in the latest docker image that causes asciidoctor-pdf
to fail when run locally (but not when run in CI).  The problem is that
the user is different when running locally, and this user does not have
write access to the HOME directory.  It appears that asciidoctor-pdf
attempts to write temporary files into HOME, so this failed with an
access permission error.

Fix this by setting HOME=/tmp when doing a local docker build.
@gmlueck gmlueck added the editorial Some purely editorial problem label Jul 2, 2024
@gmlueck
Copy link
Contributor Author

gmlueck commented Jul 2, 2024

@oddhack, FYI: This problem started appearing with the "asciidoctor-spec.20240630" image. Maybe that image incorporated a new version of asciidoctor-pdf that writes temporary files into HOME? Just bringing this to your attention in case other projects need to make a change similar to what I'm doing here.

@oddhack
Copy link
Contributor

oddhack commented Jul 2, 2024

@oddhack, FYI: This problem started appearing with the "asciidoctor-spec.20240630" image. Maybe that image incorporated a new version of asciidoctor-pdf that writes temporary files into HOME? Just bringing this to your attention in case other projects need to make a change similar to what I'm doing here.

Thanks. I see some 'Fontconfig error: No writable cache directories' warnings and HOME=/, which also affects the Vulkan build, though does not seem to prevent PDF generation in either repo. Why you're actually getting a failure, I do not know - I just get the warnings. How are you invoking Docker, and what is the error reported?

I can likely configure the image so ROOT points to /home/dockeruser (or whatever) (although Actions does some very funky stuff when setting up a container that I don't fully grok and might interfere with that). I believe you're already setting the UID/GID appropriately in the Makefile targets so you should at least not have any files owned by root hanging around. I will experiment with the image tonight.

@oddhack
Copy link
Contributor

oddhack commented Jul 2, 2024

N.b. Docker does really weird stuff with permissions. Even though it is not SUID on my system, running it from my user account defaults to the container running as its (local) root - which leaves files owned by the host root behind after the build, That feels like a massive security hole that I have just been ignoring given how the image is being used. The USER_ID and GROUP_ID explicit settings fix that.

I see the docker targets in the Makefile are sudo-ing, which just seems wrong, but maybe that's required with how docker is installed on your system?

@gmlueck
Copy link
Contributor Author

gmlueck commented Jul 2, 2024

Thanks. I see some 'Fontconfig error: No writable cache directories' warnings and HOME=/, which also affects the Vulkan build, though does not seem to prevent PDF generation in either repo. Why you're actually getting a failure, I do not know - I just get the warnings. How are you invoking Docker, and what is the error reported?

Your messages are probably the same as mine. I'm not sure if the PDF was correctly generated despite these error messages. I didn't want the SYCL spec build to print a message that says "error" even if it has no effect on the PDF.

@gmlueck
Copy link
Contributor Author

gmlueck commented Jul 2, 2024

I see the docker targets in the Makefile are sudo-ing, which just seems wrong, but maybe that's required with how docker is installed on your system?

This is how docker works. See the post-install documentation for docker. Essentially, the docker daemon always runs as root. You can either prefix all docker commands with "sudo", or you can create a special group that grants root-permission to docker users. The SYCL makefile uses the "sudo" mechanism because it eliminates one step from the docker installation requirements. It also makes the security impact more obvious.

@oddhack
Copy link
Contributor

oddhack commented Jul 2, 2024

Try SHA 4aab96a03ef292439c9bd0f972adfa29cdf838d0909b1cb4ec2a6d7b2d14a37f (asciidoctor-spec.20240702). That is identical to the previous image aside from setting HOME=/tmp.

@gmlueck
Copy link
Contributor Author

gmlueck commented Jul 2, 2024

Try SHA 4aab96a03ef292439c9bd0f972adfa29cdf838d0909b1cb4ec2a6d7b2d14a37f (asciidoctor-spec.20240702). That is identical to the previous image aside from setting HOME=/tmp.

That also seems to fix the problem. I'll probably go ahead with this PR anyway, though, because it keeps all the related things together. We had the permission problem because we changed the user ID when running in the docker container. (If we do the build with the default user of "root", then everything works fine.) Since we changed the user ID in the makefile, I think it makes sense that we add the workaround for the permission problem also in the makefile, where we can document the relationship with the user ID.

@oddhack
Copy link
Contributor

oddhack commented Jul 3, 2024

(If we do the build with the default user of "root", then everything works fine.)

The annoying part for me is that it leaves files owned by root behind in the repo (and also /tmp, now), and removing them requires another sudo.

@gmlueck
Copy link
Contributor Author

gmlueck commented Jul 3, 2024

(If we do the build with the default user of "root", then everything works fine.)

The annoying part for me is that it leaves files owned by root behind in the repo (and also /tmp, now), and removing them requires another sudo.

You mean the case when you do a local build with the default user of "root"? Yes, this is annoying because the generated output files in your local repo (outside of the container) end up being owned by root. That's why the makefile changes the user ID.

There is no problem with temporary files with root ownership being left in /tmp, though. The change in this PR only writes files to /tmp inside the container, and the container is destroyed after the build completes. Therefore, nothing is left behind in /tmp.

@oddhack
Copy link
Contributor

oddhack commented Jul 3, 2024

Running as your own UID/GID in the container is definitely the right answer, and we've sorted out the permissions problem on $HOME one way or another, so all good now AFAIK.

@gmlueck
Copy link
Contributor Author

gmlueck commented Jul 3, 2024

Merging as editorial.

@gmlueck gmlueck merged commit 8f8e6ed into KhronosGroup:SYCL-2020/master Jul 3, 2024
2 checks passed
@gmlueck gmlueck deleted the gmlueck/docker-tmp-dir branch July 10, 2024 20:38
keryell pushed a commit that referenced this pull request Sep 10, 2024
gmlueck added a commit that referenced this pull request Nov 7, 2024
Fix local docker build of PDF spec

(cherry picked from commit 8f8e6ed)
gmlueck added a commit that referenced this pull request Nov 7, 2024
Fix local docker build of PDF spec

(cherry picked from commit 8f8e6ed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Some purely editorial problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants