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

Default GIMME_TMP to /tmp/gimme-$USER #17

Merged
merged 3 commits into from
Nov 2, 2022
Merged

Default GIMME_TMP to /tmp/gimme-$USER #17

merged 3 commits into from
Nov 2, 2022

Conversation

meatballhat
Copy link
Member

@meatballhat meatballhat commented Oct 27, 2022

and exclude it from backups as with GIMME_VERSION_PREFIX.

Closes #7

Yaz Saito and others added 2 commits October 27, 2022 07:53
and also exclude it from backups as with `GIMME_VERSION_PREFIX`
@tianon
Copy link
Member

tianon commented Oct 27, 2022

My only concern with moving this out of /tmp is that we then become responsible for cleaning it out ourselves (where /tmp is generally expected to be cleaned out by the OS if we accidentally forget to), so we need to be slightly more careful about what we put in there and whether it's growing over time. 😅

@meatballhat
Copy link
Member Author

@tianon Very true! Given this solution vs., say, /tmp/gimme-of-$(whoami) (inspired by pytest?), which do you favor? (Or something else?)

@tianon
Copy link
Member

tianon commented Oct 31, 2022

$ git grep -n GIMME_TMP gimme
gimme:34:#+               GIMME_TMP - temp directory (default '${GIMME_TMP}')
gimme:347:      cat >"${GIMME_TMP}/test.go" <<'EOF'
gimme:354:      "${1}/bin/go" run "${GIMME_TMP}/test.go"
gimme:455:      local bin_tgz="${GIMME_TMP}/go${version}.${GIMME_OS}.${arch}.tar.gz"
gimme:472:      local src_tgz="${GIMME_TMP}/go${GIMME_GO_VERSION}.src.tar.gz"
gimme:547:      local dlfile="${GIMME_TMP}/known-dl"
gimme:801:: "${GIMME_TMP:=${TMPDIR:-/tmp}/gimme}"

Looking at these uses, test.go is going to be small and consistently overwritten, as is known-dl, and the go tarballs will probably be either used or reattempted, so I guess it probably doesn't make much difference.

Given we don't keep the tarballs (ie, mv $GIMME_TMP/go...tar.gz $GIMMIE_SOMEWHERE_ELSE), I don't see a strong benefit to them living inside our directory (where we do get good benefit from them being in /tmp), so I'd still personally lean towards the existing ${TMPDIR:-/tmp} as our base directory (and fixing the issue-at-hand with something like ${USER:-$(id -u)} injected), but I don't feel so strongly that I'd block any approach others feel is more correct. 👍

@tianon
Copy link
Member

tianon commented Oct 31, 2022

TLDR: I'd prefer something like : "${USER:=$(id -u)}" and ${TMPDIR:-/tmp}/gimme-$USER, but don't feel so strongly to block anything

@meatballhat
Copy link
Member Author

@tianon I like your idea better!

@meatballhat meatballhat changed the title Default GIMME_TMP to ~/.gimme/tmp Default GIMME_TMP to /tmp/gimme-$USER Nov 2, 2022
@meatballhat meatballhat requested a review from a team November 2, 2022 11:55
@meatballhat meatballhat merged commit c6fbbd8 into main Nov 2, 2022
@meatballhat meatballhat deleted the namespaced-tmp branch November 2, 2022 13:26
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.

Apply namespaced temporary files patch
2 participants