8000 Update README by aeaton-overleaf · Pull Request #213 · overleaf/clsi · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Dec 17, 2021. It is now read-only.

Update README #213

Merged
merged 14 commits into from
Mar 23, 2021
Merged

Update README #213

merged 14 commits into from
Mar 23, 2021

Conversation

aeaton-overleaf
Copy link
Contributor

This updates the README with instructions for running the CLSI in Docker using the texlive/texlive image for sibling containers.

@mans0954
Copy link
Contributor

3009 has never been the CLSI port - it's the filestore port. I must have been confused when I added that to the notes!

@mans0954
Copy link
Contributor

There are a number of environment variables missing from the list. These are the ones I found going through the setting file:

ALLOWED_COMPILE_GROUPS
ALLOWED_IMAGES
CATCH_ERRORS
COMPILE_GROUP_DOCKER_CONFIGS
COMPILES_HOST_DIR
COMPILE_SIZE_LIMIT
DOCKER_RUNNER
DOCKER_RUNTIME
FILESTORE_DOMAIN_OVERRIDE
FILESTORE_PARALLEL_FILE_DOWNLOADS
FILESTORE_PARALLEL_SQL_QUERY_LIMIT
LISTEN_ADDRESS
PROCESS_LIFE_SPAN_LIMIT_MS
SENTRY_DSN
SMOKE_TEST
SQLITE_PATH
SYNCTEX_BIN_HOST_PATH
TEXLIVE_IMAGE
TEX_LIVE_IMAGE_NAME_OVERRIDE
TEXLIVE_IMAGE_USER
TEXLIVE_OPENOUT_ANY

@mans0954
Copy link
Contributor
mans0954 commented Mar 11, 2021 •
edited
Loading

These instructions don't work as is on Linux, because the root user in the texlive image gets mapped to the root user on the host. A workaround is to do something like:

sudo chown -R mans0954:root compiles/
sudo chmod g+w -R compiles/

See this internal issue: https://github.com/overleaf/dev-environment/issues/79
And this upstream issue: moby/moby#7198

mans0954
mans0954 suggested changes Mar 11, 2021
View reviewed changes
Copy link
Contributor
@mans0954 mans0954 left a comment
8000

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for picking this up! I think the path to the default settings file should be changed. If you've got the time it would be great to add the extra env vars in to the README.

I've never found an entirely satisfactory solution to the Linux file permission mapping. In the dev environment we fudged around it by changing the uid of my Ubuntu account from 1001 to 1000 to match the tex user in our gcr.io/overleaf-ops/texlive-full image.

@aeaton-overleaf
Copy link
Contributor Author
aeaton-overleaf commented Mar 11, 2021 •
edited
Loading

These instructions don't work as is on Linux, because the root user in the texlive image gets mapped to the root user on the host.

I couldn't find a more appropriate user in the texlive/texlive image, but if there's a suggestion that works here I'd be happy to update the command.

@aeaton-overleaf
Copy link
Contributor Author

it would be great to add the extra env vars in to the README.

If you wouldn't mind adding them in a separate PR that might be better, as I'm not sure of the description needed for each one.

@jdleesmiller
Copy link
Member

sudo chown -R mans0954:root compiles/

It may be worth asking... do we need to bind mount compiles at all? I think it's basically ephemeral data. Keeping it in a volume instead of a bind mount might avoid some complexity here. (Also, I think it would be fine for the /compiles data to live in /tmp for the same reason, and making a volume for /tmp is a fairly standard thing to do.) But, that could all be separate to these README improvements!

@mans0954 mans0954 mentioned this pull request Mar 16, 2021
Merged
mans0954 and others added 2 commits March 16, 2021 15:29
@mans0954
README typos
14ece16
@aeaton-overleaf
Merge pull request #216 from overleaf/csh-ae-readme
7ec07e2 
Add missing environment variables to CLSI README.
@mans0954
Copy link
Contributor

It may be worth asking... do we need to bind mount compiles at all? I think it's basically ephemeral data. Keeping it in a volume instead of a bind mount might avoid some complexity here. (Also, I think it would be fine for the /compiles data to live in /tmp for the same reason, and making a volume for /tmp is a fairly standard thing to do.)

Currently a folder on the host (e.g. $PWD/compiles) gets mounted into CLSI and then project subfolders (e.g. $PWD/compiles/1234) get mounted into the TeX Live container. Presumably DockerRunner would need to create per-project volumes in order to achieve the same level of security as we have at the moment, as I don't think you can mount a path within a volume into a container? Possibly sharing a single volume between the CLSI and all the TeX Live containers would be acceptable in the dev environment?

But, that could all be separate to these README improvements!

Is it worth writing up an issue for this?

@aeaton-overleaf
Copy link
Contributor Author
aeaton-overleaf commented Mar 17, 2021 •
edited
Loading

I don't think you can mount a path within a volume into a container?

Right, it doesn't seem to be possible. moby/moby#32582

You could in theory mount the whole named volume somewhere without read access and symlink the project folder into somewhere with read access, but that would be a bit risky.

@jdleesmiller
Copy link
Member

mounted into the TeX Live container

Good points, thanks! I think we can forget about my comment, in that case. I have given the upstream issue a 👍 .

@mans0954
Copy link
Contributor

Good points, thanks! I think we can forget about my comment, in that case. I have given the upstream issue a .

Okay, not quite sure where to go next with this. There are some good improvements to the README in the PR, but I feel we should at least acknowledge that the instructions won't work if followed literally on most Linux setups.

@aeaton-overleaf
Copy link
Contributor Author

To make it work on Linux, is it enough to comment out the -e TEXLIVE_IMAGE_USER=root \ line? I could do that and add a comment about enabling it for macOS.

@aeaton-overleaf
Copy link
Contributor Author
aeaton-overleaf commented Mar 18, 2021 •
edited
Loading

Updated to move the TEXLIVE_IMAGE_USER line into the macOS-specific instructions.

@mans0954
Copy link
Contributor

To make it work on Linux, is it enough to comment out the -e TEXLIVE_IMAGE_USER=root \ line? I could do that and add a comment about enabling it for macOS.

Unfortunately that gives the error (HTTP code 500) server error - linux spec user: unable to find user tex: no matching entries in passwd file because TEXLIVE_IMAGE_USER defaults to tex and there is no tex user in the TeX Live image.

Possibly one could provide instructions for building an image based on Tex Live which contained an additional user. I think you would still need to make the uids of the users in the images match with the uid of the host user.

@aeaton-overleaf
Copy link
Contributor Author

If it's not currently possible to make this work on Linux then I'll propose merging this and updating it when there's a solution.

@aeaton-overleaf
Copy link
Contributor Author

@mans0954 Would documenting this workaround that you proposed earlier make sense instead?

sudo chown -R mans0954:root compiles/
sudo chmod g+w -R compiles/

@mans0954
Copy link
Contributor

@mans0954 Would documenting this workaround that you proposed earlier make sense instead?

sudo chown -R mans0954:root compiles/
sudo chmod g+w -R compiles/

Yes, I think that was what I originally had in mind!

@aeaton-overleaf
Copy link
Contributor Author

Updated with instructions for changing permissions on the compiles folder on Linux.

@mans0954
Copy link
Contributor

Updated with instructions for changing permissions on the compiles folder on Linux.

To be honest, it's a bit more complicated than that. I'll try to write something up.

mans0954
mans0954 approved these changes Mar 19, 2021
View reviewed changes
Copy link
Contributor
@mans0954 mans0954 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've written something about the permissions issue on Linux. It's complicated as some distros use 1000 as the uid for the first normal user and some use 1001.

@aeaton-overleaf aeaton-overleaf merged commit 5da7f47 into master Mar 23, 2021
@aeaton-overleaf aeaton-overleaf deleted the ae-readme branch March 23, 2021 09:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mans0954 mans0954 mans0954 approved these changes

Assignees

@mans0954 mans0954

Labels
Team-Aqua
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aeaton-overleaf @mans0954 @jdleesmiller @40thieves
0