jneen
commented
13 years ago 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.
Closed tabletcorry closed 13 years ago
jneen
commented
13 years ago 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
commented
13 years ago I will probably wrap some of these with if statements this evening, unless someone else has started work, or has objection.
jordansissel
commented
13 years ago 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 -emode 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: https://github.com/jordansissel/fpm/issues/86
tabletcorry
commented
13 years ago 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 -emode 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: https://github.com/jordansissel/fpm/issues/86
Reply to this email directly or view it on GitHub: https://github.com/jordansissel/fpm/issues/86#issuecomment-1679131
tabletcorry
commented
13 years ago See #88
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 -emode for ruby :)Survey results: