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

Write temporary files to local directory by default #10

Closed
wants to merge 2 commits into from
Closed

Conversation

stweil
Copy link
Collaborator

@stweil stweil commented Dec 4, 2019

Signed-off-by: Stefan Weil sw@weilnetz.de

Signed-off-by: Stefan Weil <sw@weilnetz.de>
@stweil stweil requested review from kba and bertsky December 4, 2019 15:56
@stweil
Copy link
Collaborator Author

stweil commented Dec 4, 2019

This fixes a build problem on one of my virtual machines which has only 1.9 GB free in the root filesystem (and so in /tmp, too), but provides a large volume for /HOME with my user data and 25 GB free disk space.

Especially torch (required by ocrd-anybaseocr) seems to need much space in TMPDIR.

Copy link
Collaborator

@bertsky bertsky left a comment

Choose a reason for hiding this comment

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

I understand the use-case and the solution is nice. But since often /tmp is in-memory or on faster disks, and automatic clean-up is included, I'd prefer this to be an opt-in, not an opt-out.

IIUC, modern Linux distributions do not set TMPDIR by default (despite it's being a POSIX requirement), but rely on the /tmp fallback. So your patch would change the default behaviour from /tmp to ./tmp.

Have you tried running your build with make ocrd-anybaseocr-crop TMPDIR=$PWD/tmp or export TMPDIR=$PWD/tmp; make ocrd-anybaseocr-crop – without the patch? (If that works, all we'd have to do is add a hint about TMPDIR in the README and for make help.)

Copy link
Member

@kba kba left a comment

Choose a reason for hiding this comment

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

I concur with @bertsky, let's make this optional and default to /tmp so you CAN do

d=$(mktemp -d -p $PWD) && \
make ocrd-anybaseocr-crop TMPDIR="$d" && \
rm -rf -- "$d"

but do not HAVE to set TMPDIR explicitly

@stweil
Copy link
Collaborator Author

stweil commented Dec 4, 2019

I used TMPDIR=$PWD/tmp make all successfully without this patch, so this modifications just makes life a little bit easier for me.

@bertsky
Copy link
Collaborator

bertsky commented Dec 4, 2019

I used TMPDIR=$PWD/tmp make all successfully without this patch, so this modifications just makes life a little bit easier for me.

Then I'd say just put export TMPDIR=path/to/ocrd_all/tmp in your ~/.bashrc or in venv/bin/activate and you're good. (We should still add this remark to make help and README, though.)

@kba
Copy link
Member

kba commented Dec 4, 2019

I added a paragraph to the README on this.

@stweil stweil closed this Dec 4, 2019
@stweil stweil deleted the tmpdir branch December 4, 2019 18:37
@stweil stweil restored the tmpdir branch December 4, 2019 18:37
@stweil stweil deleted the tmpdir branch December 4, 2019 18:40
@kba
Copy link
Member

kba commented Dec 4, 2019

Can you cherry pick UB-Mannheim@9848636 to master though?

@stweil
Copy link
Collaborator Author

stweil commented Dec 4, 2019

Already done. I also fixed a typo, so you will get a local conflict.

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

3 participants