fpm should check return values #86

tabletcorry · 2011-07-28T22:02:27Z

It appears that fpm almost never checks return values. In a quick survey of the code, only 2 out of 34 calls to system() actually captured the return value.

The direct result of this is fpm creating empty packages. As an example, if you try to make a gem into an rpm, and the gem build fails, it will keep going and make the rpm anyways. Considering the significant length of the rpm output, it is easy to miss the actual error output, if you do not scroll up.

I don't suppose there is a set -e mode for ruby :)

Survey results:

chaines@Sharpimac:fpm                                                                            (master u=)  14:54:41
$ grep system **/*.rb
lib/fpm/builder.rb:          system("gzip -d #{data_tarball}")
lib/fpm/builder.rb:            system("tar -xf #{data_tarball.gsub(/\.gz$/, "")}")
lib/fpm/builder.rb:    system("#{editor} '#{package.specfile(builddir)}'")
lib/fpm/source.rb:      system(*dir_tar) if dirs.any?
lib/fpm/source.rb:      system(*files_tar)
lib/fpm/source/dir.rb:        system(*rsync)
lib/fpm/source/dir.rb:        system("ls #{builddir}/tarbuild")
lib/fpm/source/dir.rb:    system(*["gzip", "-f", tar_path])
lib/fpm/source/gem.rb:    system(*args)
lib/fpm/source/gem.rb:    system(*["gzip", "-f", tar_path])
lib/fpm/source/npm.rb:    system(*args)
lib/fpm/source/npm.rb:    system(*["gzip", "-f", tar_path])
lib/fpm/source/python.rb:    return_value = system(self[:settings][:easy_install], "--editable", "--build-directory", @tmpdir, want_pkg)
lib/fpm/source/python.rb:      system(self[:settings][:python], "setup.py", "bdist")
lib/fpm/source/python.rb:    system("cp", dist_tar, "#{tar_path}.gz")
lib/fpm/source/rpm.rb:    system("rpm2cpio #{@rpm} | (cd #{tmpdir}; cpio -i --make-directories)")
lib/fpm/source/rpm.rb:    system(*["gzip", "-f", tar_path])
lib/fpm/source/tar.rb:    system("tar #{flags}")
lib/fpm/source/tar.rb:    system(*["gzip", "-f", tar_path])
lib/fpm/target/deb.rb:      # to ask the system about.
lib/fpm/target/deb.rb:          system("cp #{path} ./preinst")
lib/fpm/target/deb.rb:          system("cp #{path} ./postinst")
lib/fpm/target/deb.rb:          system("cp #{path} ./prerm")
lib/fpm/target/deb.rb:          system("cp #{path} ./postrm")
lib/fpm/target/deb.rb:    system("tar -zcf control.tar.gz #{control_files.map{ |f| "./#{f}" }.join(" ")}")
lib/fpm/target/deb.rb:    system("ar -qc #{params[:output]} debian-binary control.tar.gz data.tar.gz")
lib/fpm/target/rpm.rb:    ret = system(*args)
lib/fpm/target/rpm.rb:      system("mv", path, params[:output])
lib/fpm/target/solaris.rb:          system("cp #{path} ./preinstall")
lib/fpm/target/solaris.rb:          system("cp #{path} ./postinstall")
lib/fpm/target/solaris.rb:    system("gzip -d data.tar.gz");
lib/fpm/target/solaris.rb:      system("tar -xf ../data.tar");
lib/fpm/target/solaris.rb:    #system("(echo 'i pkginfo'; pkgproto data=/) > Prototype")
lib/fpm/target/solaris.rb:    system("pkgmk -o -d .")
lib/fpm/target/solaris.rb:    system("pkgtrans -s . #{params[:output]} #{name}")

The text was updated successfully, but these errors were encountered:

jneen · 2011-07-28T22:12:51Z

Even better would be to not shell out nearly as often - which is one thing @jordansissel has been working on. That way exceptions would bubble up naturally.

tabletcorry · 2011-07-28T23:14:58Z

I will probably wrap some of these with if statements this evening, unless someone else has started work, or has objection.

jordansissel · 2011-07-29T06:36:40Z

On Thu, Jul 28, 2011 at 3:02 PM, tabletcorry <
reply@reply.github.com>wrote:

It appears that fpm almost never checks return values. In a quick survey of
the code, only 2 out of 34 calls to system() actually captured the return
value.

The direct result of this is fpm creating empty packages. As an example, if
you try to make a gem into an rpm, and the gem build fails, it will keep
going and make the rpm anyways. Considering the significant length of the
rpm output, it is easy to miss the actual error output, if you do not scroll
up.

I don't suppose there is a set -e mode for ruby :)

I could certainly implement a 'set -e'-like system call to replace all 34
calls below. I'll work on cleaning up the code.

For a poor excuse, most of FPM was hacked together in anger and blind rage
without my usual code quality, love, and attention put into it. Thanks again
for doing the audit :)

-Jordan

Survey results:

chaines@Sharpimac:fpm
                       (master u=)  14:54:41
$ grep system **/*.rb
lib/fpm/builder.rb:          system("gzip -d #{data_tarball}")
lib/fpm/builder.rb:            system("tar -xf #{data_tarball.gsub(/\.gz$/,
"")}")
lib/fpm/builder.rb:    system("#{editor} '#{package.specfile(builddir)}'")
lib/fpm/source.rb:      system(*dir_tar) if dirs.any?
lib/fpm/source.rb:      system(*files_tar)
lib/fpm/source/dir.rb:        system(*rsync)
lib/fpm/source/dir.rb:        system("ls #{builddir}/tarbuild")
lib/fpm/source/dir.rb:    system(*["gzip", "-f", tar_path])
lib/fpm/source/gem.rb:    system(*args)
lib/fpm/source/gem.rb:    system(*["gzip", "-f", tar_path])
lib/fpm/source/npm.rb:    system(*args)
lib/fpm/source/npm.rb:    system(*["gzip", "-f", tar_path])
lib/fpm/source/python.rb:    return_value =
system(self[:settings][:easy_install], "--editable", "--build-directory",
@tmpdir, want_pkg)
lib/fpm/source/python.rb:      system(self[:settings][:python], "setup.py",
"bdist")
lib/fpm/source/python.rb:    system("cp", dist_tar, "#{tar_path}.gz")
lib/fpm/source/rpm.rb:    system("rpm2cpio #{@rpm} | (cd #{tmpdir}; cpio -i
--make-directories)")
lib/fpm/source/rpm.rb:    system(*["gzip", "-f", tar_path])
lib/fpm/source/tar.rb:    system("tar #{flags}")
lib/fpm/source/tar.rb:    system(*["gzip", "-f", tar_path])
lib/fpm/target/deb.rb:      # to ask the system about.
lib/fpm/target/deb.rb:          system("cp #{path} ./preinst")
lib/fpm/target/deb.rb:          system("cp #{path} ./postinst")
lib/fpm/target/deb.rb:          system("cp #{path} ./prerm")
lib/fpm/target/deb.rb:          system("cp #{path} ./postrm")
lib/fpm/target/deb.rb:    system("tar -zcf control.tar.gz
#{control_files.map{ |f| "./#{f}" }.join(" ")}")
lib/fpm/target/deb.rb:    system("ar -qc #{params[:output]} debian-binary
control.tar.gz data.tar.gz")
lib/fpm/target/rpm.rb:    ret = system(*args)
lib/fpm/target/rpm.rb:      system("mv", path, params[:output])
lib/fpm/target/solaris.rb:          system("cp #{path} ./preinstall")
lib/fpm/target/solaris.rb:          system("cp #{path} ./postinstall")
lib/fpm/target/solaris.rb:    system("gzip -d data.tar.gz");
lib/fpm/target/solaris.rb:      system("tar -xf ../data.tar");
lib/fpm/target/solaris.rb:    #system("(echo 'i pkginfo'; pkgproto data=/)
> Prototype")
lib/fpm/target/solaris.rb:    system("pkgmk -o -d .")
lib/fpm/target/solaris.rb:    system("pkgtrans -s . #{params[:output]}
#{name}")

Reply to this email directly or view it on GitHub:
#86

tabletcorry · 2011-07-29T06:39:26Z

No excuse needed. Rage code is an important part of DevOps :)
On Jul 28, 2011 11:36 PM, "jordansissel" <
reply@reply.github.com>
wrote:

On Thu, Jul 28, 2011 at 3:02 PM, tabletcorry <
reply@reply.github.com>wrote:

It appears that fpm almost never checks return values. In a quick survey
of
the code, only 2 out of 34 calls to system() actually captured the
return
value.

The direct result of this is fpm creating empty packages. As an example,
if
you try to make a gem into an rpm, and the gem build fails, it will keep
going and make the rpm anyways. Considering the significant length of the
rpm output, it is easy to miss the actual error output, if you do not
scroll
up.

I don't suppose there is a set -e mode for ruby :)

I could certainly implement a 'set -e'-like system call to replace all 34
calls below. I'll work on cleaning up the code.

For a poor excuse, most of FPM was hacked together in anger and blind rage
without my usual code quality, love, and attention put into it. Thanks
again
for doing the audit :)

-Jordan
Survey results:
chaines@Sharpimac:fpm
(master u=) 14:54:41
$ grep system **/*.rb
lib/fpm/builder.rb: system("gzip -d #{data_tarball}")
lib/fpm/builder.rb: system("tar -xf #{data_tarball.gsub(/\.gz$/,
"")}")
lib/fpm/builder.rb: system("#{editor} '#{package.specfile(builddir)}'")
lib/fpm/source.rb: system(*dir_tar) if dirs.any?
lib/fpm/source.rb: system(*files_tar)
lib/fpm/source/dir.rb: system(*rsync)
lib/fpm/source/dir.rb: system("ls #{builddir}/tarbuild")
lib/fpm/source/dir.rb: system(*["gzip", "-f", tar_path])
lib/fpm/source/gem.rb: system(*args)
lib/fpm/source/gem.rb: system(*["gzip", "-f", tar_path])
lib/fpm/source/npm.rb: system(*args)
lib/fpm/source/npm.rb: system(*["gzip", "-f", tar_path])
lib/fpm/source/python.rb: return_value =
system(self[:settings][:easy_install], "--editable", "--build-directory",
@tmpdir, want_pkg)
lib/fpm/source/python.rb: system(self[:settings][:python], "setup.py",
"bdist")
lib/fpm/source/python.rb: system("cp", dist_tar, "#{tar_path}.gz")
lib/fpm/source/rpm.rb: system("rpm2cpio #{@rpm} | (cd #{tmpdir}; cpio -i
--make-directories)")
lib/fpm/source/rpm.rb: system(*["gzip", "-f", tar_path])
lib/fpm/source/tar.rb: system("tar #{flags}")
lib/fpm/source/tar.rb: system(*["gzip", "-f", tar_path])
lib/fpm/target/deb.rb: # to ask the system about.
lib/fpm/target/deb.rb: system("cp #{path} ./preinst")
lib/fpm/target/deb.rb: system("cp #{path} ./postinst")
lib/fpm/target/deb.rb: system("cp #{path} ./prerm")
lib/fpm/target/deb.rb: system("cp #{path} ./postrm")
lib/fpm/target/deb.rb: system("tar -zcf control.tar.gz
#{control_files.map{ |f| "./#{f}" }.join(" ")}")
lib/fpm/target/deb.rb: system("ar -qc #{params[:output]} debian-binary
control.tar.gz data.tar.gz")
lib/fpm/target/rpm.rb: ret = system(*args)
lib/fpm/target/rpm.rb: system("mv", path, params[:output])
lib/fpm/target/solaris.rb: system("cp #{path} ./preinstall")
lib/fpm/target/solaris.rb: system("cp #{path} ./postinstall")
lib/fpm/target/solaris.rb: system("gzip -d data.tar.gz");
lib/fpm/target/solaris.rb: system("tar -xf ../data.tar");
lib/fpm/target/solaris.rb: #system("(echo 'i pkginfo'; pkgproto data=/)
> Prototype")
lib/fpm/target/solaris.rb: system("pkgmk -o -d .")
lib/fpm/target/solaris.rb: system("pkgtrans -s . #{params[:output]}
#{name}")
Reply to this email directly or view it on GitHub:
#86
Reply to this email directly or view it on GitHub:
#86 (comment)

Basically, its a replacement for system. If the command fails, then it raises an exception and prints out the entire command that was attempted. This will prevent issues where fpm finished (with return code 0) and produces an artifact with real size (>0 bytes) but the artifact is worthless as it contains nothing. Fixes jordansissel#86

tabletcorry · 2011-08-16T22:16:17Z

See #88

Basically, its a replacement for system. If the command fails, then it raises an exception and prints out the entire command that was attempted. This will prevent issues where fpm finished (with return code 0) and produces an artifact with real size (>0 bytes) but the artifact is worthless as it contains nothing. Fixes jordansissel#86

Basically, its a replacement for system. If the command fails, then it raises an exception and prints out the entire command that was attempted. This will prevent issues where fpm finished (with return code 0) and produces an artifact with real size (>0 bytes) but the artifact is worthless as it contains nothing. Fixes #86

tabletcorry closed this as completed Aug 16, 2011

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fpm should check return values #86

fpm should check return values #86

tabletcorry commented Jul 28, 2011

jneen commented Jul 28, 2011

tabletcorry commented Jul 28, 2011

jordansissel commented Jul 29, 2011

tabletcorry commented Jul 29, 2011

tabletcorry commented Aug 16, 2011

fpm should check return values #86

fpm should check return values #86

Comments

tabletcorry commented Jul 28, 2011

jneen commented Jul 28, 2011

tabletcorry commented Jul 28, 2011

jordansissel commented Jul 29, 2011

tabletcorry commented Jul 29, 2011

tabletcorry commented Aug 16, 2011