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

Support an extension to tmpfile #1735

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented May 3, 2022

Sometimes tools expect a certain extension or it is just cleaner to have one. This adds an optional parameter which keeps the API compatible.

@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.10 ⚠️

Comparison is base (44c98b1) 74.65% compared to head (d8eb5fd) 74.55%.

❗ Current head d8eb5fd differs from pull request most recent head 37dd194. Consider uploading reports for the commit 37dd194 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1735      +/-   ##
==========================================
- Coverage   74.65%   74.55%   -0.10%     
==========================================
  Files          81       81              
  Lines        4494     4508      +14     
==========================================
+ Hits         3355     3361       +6     
- Misses       1139     1147       +8     
Impacted Files Coverage Δ
lib/beaker/host/pswindows/file.rb 73.68% <33.33%> (-8.67%) ⬇️
lib/beaker/host.rb 78.64% <50.00%> (-1.28%) ⬇️
lib/beaker/host/unix/file.rb 90.90% <50.00%> (ø)
lib/beaker/host/windows/file.rb 76.66% <50.00%> (ø)
lib/beaker/host/freebsd/pkg.rb 84.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

There seems to be some portability issue. If we can live without the guaranteed uniqueness of the filenames crated with mktemp, a simple workaround might be to create a temporary file as before and rename it after that?

def tmpfile(name = '')
execute("rndnum=${RANDOM} && touch /tmp/#{name}.${rndnum} && echo /tmp/#{name}.${rndnum}")
def tmpfile(name = '', extension = nil)
execute("rndnum=${RANDOM} && touch /tmp/#{name}.${rndnum} && echo /tmp/#{name}.${rndnum}#{extension}")
Copy link
Member

Choose a reason for hiding this comment

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

This looks bad (regarding the uniqueness of the generated temporary file name) but I guess AIX is that bad 🤷

Suggested change
execute("rndnum=${RANDOM} && touch /tmp/#{name}.${rndnum} && echo /tmp/#{name}.${rndnum}#{extension}")
execute("rndnum=${RANDOM} && touch /tmp/#{name}.${rndnum}#{extension} && echo /tmp/#{name}.${rndnum}#{extension}")

def tmpfile(name = '')
execute("mktemp -t #{name}.XXXXXX")
def tmpfile(name = '', extension = nil)
execute("mktemp -t #{name}.XXXXXX#{extension}")
Copy link
Member

Choose a reason for hiding this comment

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

This is not portable, for example the FreeBSD man page for mktemp(1) says:

The template may be any file name with some number of ‘Xs’ appended to it, for example /tmp/temp.XXXX. The trailing ‘Xs’ are replaced with the current process number and/or a unique letter combination.

And indeed, it does not do the right thing on FreeBSD:

romain@zappy ~ % mktemp -t foo.XXXX.gz
/tmp/foo.XXXX.gz.uAo9BCUu

I am not sure there is something guaranteed to be portable given mktemp is not defined by POSIX. The GNU/Linux and FreeBSD man pages at least contain references to function that support this:

The mkstemps() and mkostemps() functions act the same as mkstemp() and mkostemp() respectively, except they permit a suffix to exist in the template. The template should be of the form /tmp/tmpXXXXXXsuffix. The mkstemps() and mkostemps() function are told the length of the suffix string.

Copy link
Member Author

@ekohl ekohl Jul 5, 2022

Choose a reason for hiding this comment

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

Thanks for that insight. I wonder what would be best to do here. Given Windows does not implement it either, would it be OK to mention it as a potential compatibility issue in a param documentation? I mean, in the end it still gives you a unique filename.

Edit: https://hauweele.net/~gawen/blog/?p=2529 suggests mktemp -d to create a directory and then create a file with a static name. I guess that could be implemented for FreeBSD while using GNU mktemp's feature on Linux. However, it is a risk since it's multiple code paths.

Copy link
Member

Choose a reason for hiding this comment

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

That looks great!

Copy link
Member

Choose a reason for hiding this comment

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

Wait wait wait, why don't we use Tempfile from stdlib which already has support for this:

The basename parameter is used to determine the name of the temporary file. You can either pass a String or an Array with 2 String elements. In the former form, the temporary file’s base name will begin with the given string. In the latter form, the temporary file’s base name will begin with the array’s first element, and end with the second element. For example:

file = Tempfile.new('hello')
file.path  # => something like: "/tmp/hello2843-8392-92849382--0"
         
# Use the Array form to enforce an extension in the filename:
file = Tempfile.new(['hello', '.jpg'])
file.path  # => something like: "/tmp/hello2843-8392-92849382--0.jpg"

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the Ruby part is executed on the host, not the target. You may need to do some wacky things to get Ruby to execute on the target host and in beaker we can't assume it's present since Puppet may not have been bootstrapped.

@ekohl
Copy link
Member Author

ekohl commented Oct 25, 2022

@smortex would you be OK with merging this?

Sometimes tools expect a certain extension or it is just cleaner to have
one. This adds an optional parameter which keeps the API compatible.
@bastelfreak bastelfreak merged commit 323ee5e into voxpupuli:master Apr 28, 2023
6 checks passed
@ekohl ekohl added enhancement and removed bug labels Apr 28, 2023
@ekohl ekohl deleted the tmpfile-extension branch April 28, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants