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

freeBSD broke between v1.11.0 and v1.13.1 #1844

Open
claytonjwong opened this issue Oct 29, 2021 · 8 comments
Open

freeBSD broke between v1.11.0 and v1.13.1 #1844

claytonjwong opened this issue Oct 29, 2021 · 8 comments

Comments

@claytonjwong
Copy link
Contributor

claytonjwong commented Oct 29, 2021

Hi, did this code change make it into the v1.13.1 release?
If so, then maybe it breaks freeBSD packaging: 63fdb94


Test Performed: I performed the following 2 tests back to back within freeBSD 12 on a vagrant instance on my localhost mac:

  1. ❌ Use fpm v1.13.1 to show incorrect package manifest, ie. each file is missing a / prefix 😵
[vagrant@freebsd12 ~/package]$ tar tf speedtest-1.2.3.txz
+COMPACT_MANIFEST
+MANIFEST
speedtest
speedtest.5
speedtest.md
  1. ✅ Use fpm v1.11.0 on the same exact files to show correct package manifest, ie. each file has a / prefix 😊
+COMPACT_MANIFEST
+MANIFEST
/speedtest
/speedtest.5
/speedtest.md

Full Test Input/Output:

[vagrant@freebsd12 ~/package]$ fpm --verbose -s dir -t freebsd -n speedtest -v 1.2.3 -a amd64 -m "support@ookla" --package . .=/
Setting workdir {:workdir=>"/tmp", :level=>:info}
Setting from flags: architecture=amd64 {:level=>:info}
Setting from flags: epoch= {:level=>:info}
Setting from flags: iteration= {:level=>:info}
Setting from flags: maintainer=support@ookla {:level=>:info}
Setting from flags: name=speedtest {:level=>:info}
Setting from flags: url=http://example.com/no-uri-given {:level=>:info}
Setting from flags: version=1.2.3 {:level=>:info}
Converting dir to freebsd {:level=>:info}
Created package {:path=>"./speedtest-1.2.3.txz"}
[vagrant@freebsd12 ~/package]$ tar tf speedtest-1.2.3.txz
+COMPACT_MANIFEST
+MANIFEST
speedtest
speedtest.5
speedtest.md
[vagrant@freebsd12 ~/package]$ fpm --version
1.13.1
[vagrant@freebsd12 ~/package]$ sudo gem uninstall fpm

Select gem to uninstall:
 1. fpm-1.11.0
 2. fpm-1.13.1
 3. All versions
> 2
Successfully uninstalled fpm-1.13.1
[vagrant@freebsd12 ~/package]$ fpm --version
Doing `require 'backports'` is deprecated and will not load any backport in the next major release.
Require just the needed backports instead, or 'backports/latest'.
1.11.0
[vagrant@freebsd12 ~/package]$ rm speedtest-1.2.3.txz
[vagrant@freebsd12 ~/package]$ fpm --verbose -s dir -t freebsd -n speedtest -v 1.2.3 -a amd64 -m "support@ookla" --package . .=/
Doing `require 'backports'` is deprecated and will not load any backport in the next major release.
Require just the needed backports instead, or 'backports/latest'.
Setting workdir {:workdir=>"/tmp", :level=>:info}
Setting from flags: architecture=amd64 {:level=>:info}
Setting from flags: epoch= {:level=>:info}
Setting from flags: iteration= {:level=>:info}
Setting from flags: maintainer=support@ookla {:level=>:info}
Setting from flags: name=speedtest {:level=>:info}
Setting from flags: url=http://example.com/no-uri-given {:level=>:info}
Setting from flags: version=1.2.3 {:level=>:info}
Converting dir to freebsd {:level=>:info}
DEPRECATION NOTICE: XZ::StreamWriter#close will automatically close the wrapped IO in the future. Use #finish to prevent that.
/usr/local/lib/ruby/gems/2.7/gems/ruby-xz-0.2.3/lib/xz/stream_writer.rb:185:in `initialize'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.11.0/lib/fpm/package/freebsd.rb:85:in `new'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.11.0/lib/fpm/package/freebsd.rb:85:in `block in output'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.11.0/lib/fpm/package/freebsd.rb:84:in `open'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.11.0/lib/fpm/package/freebsd.rb:84:in `output'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.11.0/lib/fpm/command.rb:487:in `execute'
	/usr/local/lib/ruby/gems/2.7/gems/clamp-1.0.1/lib/clamp/command.rb:68:in `run'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.11.0/lib/fpm/command.rb:574:in `run'
	/usr/local/lib/ruby/gems/2.7/gems/clamp-1.0.1/lib/clamp/command.rb:133:in `run'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.11.0/bin/fpm:7:in `<top (required)>'
	/usr/local/bin/fpm:23:in `load'
	/usr/local/bin/fpm:23:in `<main>'
Created package {:path=>"./speedtest-1.2.3.txz"}
[vagrant@freebsd12 ~/package]$ tar tf speedtest-1.2.3.txz
+COMPACT_MANIFEST
+MANIFEST
/speedtest
/speedtest.5
/speedtest.md
[vagrant@freebsd12 ~/package]$ fpm --version
Doing `require 'backports'` is deprecated and will not load any backport in the next major release.
Require just the needed backports instead, or 'backports/latest'.
1.11.0
@claytonjwong
Copy link
Contributor Author

Actually, I just tested v1.13.0 and it is also broken:

[vagrant@freebsd12 ~/package]$ sudo gem install fpm -v 1.13.0
Fetching fpm-1.13.0.gem
/usr/local/lib/ruby/site_ruby/2.7/rubygems/package.rb:509: warning: Using the last argument as keyword parameters is deprecated
Successfully installed fpm-1.13.0
Parsing documentation for fpm-1.13.0
Installing ri documentation for fpm-1.13.0
Done installing documentation for fpm after 0 seconds
1 gem installed
[vagrant@freebsd12 ~/package]$ rm speedtest-1.2.3.txz
[vagrant@freebsd12 ~/package]$ fpm --version
1.13.0
[vagrant@freebsd12 ~/package]$ fpm --verbose -s dir -t freebsd -n speedtest -v 1.2.3 -a amd64 -m "support@ookla" --package . .=/
Setting workdir {:workdir=>"/tmp", :level=>:info}
Setting from flags: architecture=amd64 {:level=>:info}
Setting from flags: epoch= {:level=>:info}
Setting from flags: iteration= {:level=>:info}
Setting from flags: maintainer=support@ookla {:level=>:info}
Setting from flags: name=speedtest {:level=>:info}
Setting from flags: url=http://example.com/no-uri-given {:level=>:info}
Setting from flags: version=1.2.3 {:level=>:info}
Converting dir to freebsd {:level=>:info}
Created package {:path=>"./speedtest-1.2.3.txz"}
[vagrant@freebsd12 ~/package]$ tar tf speedtest-1.2.3.txz
+COMPACT_MANIFEST
+MANIFEST
speedtest
speedtest.5
speedtest.md

I'll see if I can figure out when this broke and what's going on here 👻

@claytonjwong
Copy link
Contributor Author

So v1.12.0 works OK too, so something broke in v1.13.0:

[vagrant@freebsd12 ~/package]$ fpm --verbose -s dir -t freebsd -n speedtest -v 1.2.3 -a amd64 -m "support@ookla" --package . .=/
Setting workdir {:workdir=>"/tmp", :level=>:info}
Setting from flags: architecture=amd64 {:level=>:info}
Setting from flags: epoch= {:level=>:info}
Setting from flags: iteration= {:level=>:info}
Setting from flags: maintainer=support@ookla {:level=>:info}
Setting from flags: name=speedtest {:level=>:info}
Setting from flags: url=http://example.com/no-uri-given {:level=>:info}
Setting from flags: version=1.2.3 {:level=>:info}
Converting dir to freebsd {:level=>:info}
DEPRECATION NOTICE: XZ::StreamWriter#close will automatically close the wrapped IO in the future. Use #finish to prevent that.
/usr/local/lib/ruby/gems/2.7/gems/ruby-xz-0.2.3/lib/xz/stream_writer.rb:185:in `initialize'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.12.0/lib/fpm/package/freebsd.rb:85:in `new'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.12.0/lib/fpm/package/freebsd.rb:85:in `block in output'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.12.0/lib/fpm/package/freebsd.rb:84:in `open'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.12.0/lib/fpm/package/freebsd.rb:84:in `output'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.12.0/lib/fpm/command.rb:487:in `execute'
	/usr/local/lib/ruby/gems/2.7/gems/clamp-1.0.1/lib/clamp/command.rb:68:in `run'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.12.0/lib/fpm/command.rb:574:in `run'
	/usr/local/lib/ruby/gems/2.7/gems/clamp-1.0.1/lib/clamp/command.rb:133:in `run'
	/usr/local/lib/ruby/gems/2.7/gems/fpm-1.12.0/bin/fpm:7:in `<top (required)>'
	/usr/local/bin/fpm:23:in `load'
	/usr/local/bin/fpm:23:in `<main>'
Created package {:path=>"./speedtest-1.2.3.txz"}
[vagrant@freebsd12 ~/package]$ tar tf speedtest-1.2.3.txz
+COMPACT_MANIFEST
+MANIFEST
/speedtest
/speedtest.5
/speedtest.md
[vagrant@freebsd12 ~/package]$ fpm --version
1.12.0

@claytonjwong
Copy link
Contributor Author

claytonjwong commented Oct 29, 2021

I was hoping the / suffix on the package parameter would help, but is makes no difference:

Previous command with . as relative package path: fpm --verbose -s dir -t freebsd -n speedtest -v 1.2.3 -a amd64 -m "support@ookla" --package . .=/

Current command with ./ as relative package path: fpm --verbose -s dir -t freebsd -n speedtest -v 1.2.3 -a amd64 -m "support@ookla" --package ./ .=/

[vagrant@freebsd12 ~/package]$ rm speedtest-1.2.3.txz
[vagrant@freebsd12 ~/package]$ fpm --verbose -s dir -t freebsd -n speedtest -v 1.2.3 -a amd64 -m "support@ookla" --package ./ .=/
Setting workdir {:workdir=>"/tmp", :level=>:info}
Setting from flags: architecture=amd64 {:level=>:info}
Setting from flags: epoch= {:level=>:info}
Setting from flags: iteration= {:level=>:info}
Setting from flags: maintainer=support@ookla {:level=>:info}
Setting from flags: name=speedtest {:level=>:info}
Setting from flags: url=http://example.com/no-uri-given {:level=>:info}
Setting from flags: version=1.2.3 {:level=>:info}
Converting dir to freebsd {:level=>:info}
Created package {:path=>"./speedtest-1.2.3.txz"}
[vagrant@freebsd12 ~/package]$ tar tf speedtest-1.2.3.txz
+COMPACT_MANIFEST
+MANIFEST
speedtest
speedtest.5
speedtest.md
[vagrant@freebsd12 ~/package]$

@claytonjwong
Copy link
Contributor Author

claytonjwong commented Oct 29, 2021

🎯 Removal of this previous code most definitely negatively impacts freeBSD packages!...

💥 ab4eb18#diff-3cef554cb7a0554641ae67e83e6adfbe20bdfb8dcd7abe182e3f757649e9e481L94

commit ab4eb18b5fe41045a6956313010375e8a74e6547
Author: Jordan Sissel <jls@semicomplete.com>
Date:   Fri Jun 18 23:49:30 2021 -0700

@jordansissel
Copy link
Owner

@claytonjwong, nice work on the investigation! In #1796, I had to change how freebsd packages are built, and I think we can make it better.

https://github.com/jordansissel/fpm/pull/1796/files#diff-3cef554cb7a0554641ae67e83e6adfbe20bdfb8dcd7abe182e3f757649e9e481R89-R90

The change removed a dependency on the ruby xz library which required ffi, and we can't use ffi safely anymore (due to reasons discussed in #1795. The packages now use "tar -Jcf" to create an xz-compressed tar file, but due to differences in gnu tar and bsd tar, I'm not sure we can still invoke tar and have the correct freebsd package output. I do have an idea though!

I believe we can fix this by restoring some of the old behavior:

  1. Restore use of FPM::Util::TarWriter to produce the tarball, basically restoring this code removed by Remove FFI as a dependency #1796 - https://github.com/jordansissel/fpm/pull/1796/files#diff-3cef554cb7a0554641ae67e83e6adfbe20bdfb8dcd7abe182e3f757649e9e481L86-L95
  2. shell out with safesystem() to run xz to compress it.

I may not have time this week to work on it as I have family in town.

@jordansissel
Copy link
Owner

I had to change how freebsd packages are built

For clarity, I had hoped my change still generated correct freebsd packages, but as you report, it does not! Oops 🤦

I even see a comment where I was unsure if this would break things:

# Note: This will include top-level files like "/usr/bin/foo" listed in the tar as "usr/bin/fo" without
# a leading slash. I don't know if this has any negative impact on freebsd packages.

I think we can fix this as described in my previous comment, and we can also include a test case that makes sure freebsd packages have a correct file structure (files have a / prefix, etc)

@claytonjwong
Copy link
Contributor Author

Thanks 👍

@jordansissel
Copy link
Owner

In my haste, I didn't notice this was already fixed but not yet released. #1811 reported a similar issue and #1812 fixed it. I'll still write some test cases to see if I can find any other issues and also keep this fixed in the future.

jordansissel added a commit that referenced this issue Oct 30, 2021
- Files in the tarball should begin with / (#1811, #1844)
- Assert certain top-level manifest fields
- Assert manifest files are present

Idea from #1844
jordansissel added a commit that referenced this issue Oct 30, 2021
- Files in the tarball should begin with / (#1811, #1844)
- Assert certain top-level manifest fields
- Assert manifest files are present

Idea from #1844
jordansissel added a commit that referenced this issue Oct 30, 2021
- Files in the tarball should begin with / (#1811, #1844)
- Assert certain top-level manifest fields
- Assert manifest files are present

Idea from #1844
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

No branches or pull requests

2 participants