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

use temp directory as destination directory for copy command #3163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olamilekan000
Copy link
Contributor

@olamilekan000 olamilekan000 commented Jan 27, 2025

Change switches local tests from using $HOME/lima-hostname to a 
temporary directory to prevent conflicts with user-created files 
and permission errors when writing to it. 

Fixes #3162

hack/test-templates.sh Outdated Show resolved Hide resolved
@olamilekan000 olamilekan000 force-pushed the use-temp-directory-for-test branch from ac3d2da to a9bd54b Compare January 28, 2025 15:07
hack/test-templates.sh Outdated Show resolved Hide resolved
@olamilekan000 olamilekan000 force-pushed the use-temp-directory-for-test branch from a9bd54b to 7c73d05 Compare January 28, 2025 19:28
@olamilekan000 olamilekan000 requested a review from nirs January 28, 2025 19:33
@@ -192,10 +192,11 @@ if [ "$got" != "$expected" ]; then
fi

INFO "Testing limactl copy command"
tmpfile="$HOME/lima-hostname"
tmpdir=$(mktemp -d /var/tmp/lima-test-templates.XXXXXX)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tmpdir=$(mktemp -d /var/tmp/lima-test-templates.XXXXXX)
tmpdir="$(mktemp -d /var/tmp/lima-test-templates.XXXXXX)"

Copy link
Member

Choose a reason for hiding this comment

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

Also, why /var/tmp, not /tmp?

Copy link
Member

Choose a reason for hiding this comment

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

On macOS, /var/tmp seems almost unused except the following files;

$ ls -la /var/tmp/
total 264
drwxrwxrwt   5 root           wheel     160  1 29 01:06 ./
drwxr-xr-x  34 root           wheel    1088  1 28 18:39 ../
-rw-r--r--   1 _windowserver  wheel  102400  1 28 18:39 cbrgbc_1.sqlite
-rw-r--r--   1 _windowserver  wheel   32768  1 28 18:39 cbrgbc_1.sqlite-shm
-rw-r--r--   1 _windowserver  wheel       0  1 28 18:39 cbrgbc_1.sqlite-wal

(Off-topic: what is "cbrgbc"? 🤔)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, why /var/tmp, not /tmp?

I think any works since we just need a space where any user can create a file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On macOS, /var/tmp seems almost unused except the following files;

$ ls -la /var/tmp/
total 264
drwxrwxrwt   5 root           wheel     160  1 29 01:06 ./
drwxr-xr-x  34 root           wheel    1088  1 28 18:39 ../
-rw-r--r--   1 _windowserver  wheel  102400  1 28 18:39 cbrgbc_1.sqlite
-rw-r--r--   1 _windowserver  wheel   32768  1 28 18:39 cbrgbc_1.sqlite-shm
-rw-r--r--   1 _windowserver  wheel       0  1 28 18:39 cbrgbc_1.sqlite-wal

(Off-topic: what is "cbrgbc"? 🤔)

not sure what it is but it's definitely created by sqlite. It could be for macOS specific configs

Copy link
Member

Choose a reason for hiding this comment

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

The advantage of /var/tmp is that this is typically a standard file system on any platform. /tmp is is tmpfs on some Linux distros, which has special issues (direct I/O not supported, SEEK_HOLE/DATA very slow, etc). For our use case it should not matter.

Maybe we should just use $TMPDIR?

On macOS this is the safe user tmp directory used by most programs:

% echo $TMPDIR 
/var/folders/qs/_z10qgxn3gsfw8vmkr9b3s4w0000gn/T/

It can break if someone modify the env in a stupid way, but if they do they get what they deserve.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, $TMPDIR seems better

Copy link
Member

Choose a reason for hiding this comment

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

On macOS, /var/tmp seems almost unused except the following files;

On macOS /tmp is also cleaned automatically: files not accessed for 3 days are deleted. The same is done for $TMPDIR. I don't think /var/tmp is cleaned automatically, but am not sure.

(Off-topic: what is "cbrgbc"? 🤔)

Comic Book Reader …? 🤣

Copy link
Member

Choose a reason for hiding this comment

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

(Probably "cb" = "CoreBrightness", "rgbc" = "Red, Green, Blue, and Clear". The file is used by /System/Library/HIDPlugins/ColourSensorFilterPlugin.plugin/Contents/MacOS/ColourSensorFilterPlugin)

@olamilekan000 olamilekan000 force-pushed the use-temp-directory-for-test branch from 7c73d05 to 09a26d0 Compare January 29, 2025 01:07
@AkihiroSuda AkihiroSuda added this to the v1.1.0 (tentative) milestone Jan 29, 2025
@@ -192,10 +192,11 @@ if [ "$got" != "$expected" ]; then
fi

INFO "Testing limactl copy command"
tmpfile="$HOME/lima-hostname"
tmpdir="$(mktemp -d /var/tmp/lima-test-templates.XXXXXX)"
Copy link
Member

Choose a reason for hiding this comment

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

Based on the discussion, we should use here:

tmpdir="$(mktemp -d ${TMPDIR:-/tmp}/lima-test-templates.XXXXXX)"

@nirs nirs self-requested a review January 30, 2025 13:31
@olamilekan000 olamilekan000 force-pushed the use-temp-directory-for-test branch from 09a26d0 to 0cd74d9 Compare January 30, 2025 18:25
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.

Use Temporary Directory Instead of $HOME/lima-hostname for Tests
4 participants