Re: [NEW] archivers/lrzip
Stuart Henderson wrote:
> On 2022/06/19 16:48, Rubén Llorente wrote:
> > On Tue, Jun 14, 2022 at 10:35:13PM +0200, Omar Polo wrote:
> > > the part of the getrlimit could be simplified, see the version in the
> > > updated tarball attached.
> >
> > I am including a new version with this message. With this version of the
> > patch, the RAM declared as detected will be:
> >
> > - The physical RAM (without swap) in case the rlimit is bigger than the
> > actual existing physical RAM.
> >
> > - The max RAM permisible by rlimit is the amount of RAM present is
> > bigger than the limit.
> >
> > This will allow lrzip to heuristically set its compression window
> > lengths within reasonable limits (which is the point of the get_ram
> > function).
>
> This is looking reasonably sane now. OK sthen@. I would add @sample:
imported with sthen@ tweak to the PLIST, thanks :)
> --- PLIST.origMon Jun 20 10:04:44 2022
> +++ PLIST Mon Jun 20 10:04:57 2022
> @@ -35,4 +35,6 @@ share/doc/lrzip/lzma/lzma.txt
> share/doc/lrzip/magic.header.txt
> share/doc/pkg-readmes/${PKGSTEM}
> share/examples/lrzip/
> +@sample ${SYSCONFDIR}/lrzip/
> share/examples/lrzip/lrzip.conf.example
> +@sample ${SYSCONFDIR}/lrzip/lrzip.conf
Re: [NEW] archivers/lrzip
On 2022/06/19 16:48, Rubén Llorente wrote:
> On Tue, Jun 14, 2022 at 10:35:13PM +0200, Omar Polo wrote:
> > the part of the getrlimit could be simplified, see the version in the
> > updated tarball attached.
>
> I am including a new version with this message. With this version of the
> patch, the RAM declared as detected will be:
>
> - The physical RAM (without swap) in case the rlimit is bigger than the
> actual existing physical RAM.
>
> - The max RAM permisible by rlimit is the amount of RAM present is
> bigger than the limit.
>
> This will allow lrzip to heuristically set its compression window
> lengths within reasonable limits (which is the point of the get_ram
> function).
This is looking reasonably sane now. OK sthen@. I would add @sample:
--- PLIST.orig Mon Jun 20 10:04:44 2022
+++ PLIST Mon Jun 20 10:04:57 2022
@@ -35,4 +35,6 @@ share/doc/lrzip/lzma/lzma.txt
share/doc/lrzip/magic.header.txt
share/doc/pkg-readmes/${PKGSTEM}
share/examples/lrzip/
+@sample ${SYSCONFDIR}/lrzip/
share/examples/lrzip/lrzip.conf.example
+@sample ${SYSCONFDIR}/lrzip/lrzip.conf
Re: [NEW] archivers/lrzip
Rubén Llorente wrote:
> Has anybody done any testing with this port? Does anybody think it is
> ripe for import?
few comments:
SHARED_LIBS is not needed anymore as it doesn't install a shared library
anymore.
WANTLIB is wrong. What I often do is just comment the wantlibs in the
makefile and then run `make port-lib-check-depends' which will complain
loudly about the missing libs but also provide an easy to copy-paste
list of sorted and formatted wantlibs.
CONFIGURE_STYLE can be further simplified to 'autoreconf', as that
implies gnu already.
if it were me, i'd use GH_COMMIT to fetch the version including the CVE
fix: we're currently using a patch which fixes the CVE _and_ raises
rlimit. It's easy in the future whan tweaking one or the other things
to mess up and do damages. just my opinion thought.
the part of the getrlimit could be simplified, see the version in the
updated tarball attached. (by the way, including a header in the middle
of a function hardly works.) Instead of trying to fit the openbsd bit
inside the linux one, and since there is already an ifdef'd block for
__APPLE__, why don't simply add another block for OpenBSD? The patch is
shorter, IMHO more readable and also probably easier to get it
upstreamed. I've not run-time tested it, also you may want to avoid
fatal_return and fall back to some other value maybe.
these are the changes on top of your makefile:
--- Makefile.orig Tue Jun 14 21:50:44 2022
+++ MakefileTue Jun 14 21:56:32 2022
@@ -4,8 +4,6 @@
GH_PROJECT = lrzip
GH_TAGNAME = v0.651
-SHARED_LIBS = lrzip 0.0
-
CATEGORIES = archivers
MAINTAINER = Ruben Llorente
@@ -13,7 +11,7 @@
# GPLv2+
PERMIT_PACKAGE = Yes
-WANTLIB = ${COMPILER_LIBCXX} bz2 c lzo2 m pthread z lz4
+WANTLIB += ${COMPILER_LIBCXX} bz2 c lz4 lzo2 m z
COMPILER = base-clang ports-gcc
@@ -23,7 +21,7 @@
RUN_DEPENDS = shells/bash
-CONFIGURE_STYLE = gnu autoreconf
+CONFIGURE_STYLE = autoreconf
CONFIGURE_ARGS = --disable-doc
CONFIGURE_ENV =CPPFLAGS="-I${LOCALBASE}/include" \
LDFLAGS="-L${LOCALBASE}/lib"
with these I think the port looks fine and i would ok it if someone else
wants to import it. (i haven't tested it a runtime)
Cheers,
Omar Polo
lrzip.tar.gz
Description: GNU Zip compressed data
Re: [NEW] archivers/lrzip
On Fri, 27 May 2022 23:02:46 +0200 Rubén Llorente wrote: > I have also shamelessly copied the mechanism sort uses for rising its > RDATA_LIMIT and tests so far look good. Still I have to figure out how > to do the same thing with the stack size. > > Also, how portable are setrlimit and getrlimit? Eventually we should get > this patch submitted upstream. Not portable: other Unix clones have setrlimit, but the meaning of RDATA_LIMIT is different. In some systems, RDATA_LIMIT only limits sbrk(2), so one can bypass the limit by using anonymous mmap(2) to allocate memory. OpenBSD is more strict, because our RDATA_LIMIT counts both sbrk(2) and anonymous mmap(2). malloc(3) probably uses anonymous mmap(2), so those other systems allow malloc(3) to take gigabytes of memory from anonymous mmap(2) while ignoring RDATA_LIMIT. For stack size, I'm not sure. If you are on the main thread, try RLIMIT_STACK. If you are on another thread, try pthread_attr_setstacksize(3).
Re: [NEW] archivers/lrzip
Rubén Llorente wrote: > On Fri, May 20, 2022 at 07:35:14PM +0200, Omar Polo wrote: > > The good news is that with the above changes the ports looks good IMHO. > > The bad news is that now the builds fails (link error in the embedded > > lzma copy) so it needs more work (using lzma from ports if possible) > > > > I'm attaching a tarball with above suggestions included. > > > > Cheers, > > > > Omar Polo > > > > Hello, > > I am testing the new iteration of the port and so far the bundled lzma > is not crashing on me. I can't reproduce the build failure either. Now it seems to correctly pick up lz4 from ports automatically \o/ > On the other hand I have already included the security fix in my local > version of the port. i'm attaching another version: - LIB_DEPENDS and WANTLIB fixed (was lacking lz4) - plist updated (now that uses lz4 doesn't install the library anymore) - uses the commit with the CVE-2018-5786 fix it still probably needs something to limit the amount of ram it tries to allocate, see the other mail by Stuart. I agree with him that this should use getrlimit(RLIMIT_DATA) instead of physmem. lrzip.tar.gz Description: GNU Zip compressed data
Re: [NEW] archivers/lrzip
On 2022/05/26 20:51, Rubén Llorente wrote: > On Sat, May 21, 2022 at 10:48:33AM +0100, Stuart Henderson wrote: > > how is it detecting how much memory to use? if it's looking at available > > physical memory without considering resource limits, could that be changed? > > > > there is precedent for programs to adjust RLIMIT_DATA from the default to > > the max available within the current limit if they know they're likely > > to need it - see e.g. src/usr.bin/sort/sort.c. > > > > if it's possible to automate this within reasonable bounds, this would > > be better than asking the user to adjust login.conf themselves. > > > > > > Hello, > > lrzip's resource consumption is just brutal. If you try to compress 4 Gb > worth of data it will try, by default, to use 4 GB of RAM or all > available physical memory, whatever is the smallest. https://github.com/ckolivas/lrzip/blob/master/lrzip.c#L100= So it ought to consider "memory available to the process" based on getrlimit(RLIMIT_DATA, ...) instead of "available physical memory" (Yet) another program that thinks it has a whole machine to itself.. > I personally use lrzip for massive archives (+16 GB) and, judging by > Kolivas' documentation, his idea is for lrzip to be used that way. > > Can we patch the port to do the RLIMIT_DATA trick? Maybe. I'd be > surprised if that was enough for using lrzip as intended, though :-) btw the default datasize limit for a member of "staff" class on amd64 is already 4GB. > I will play a bit with it taking Omar's advice into consideration and > try to come up with a better iteration of this port. > > Cheers! > > -- > OpenPGP Key Fingerprint: > 543F EB89 7FDE 8E33 AFF7 E794 E4AB 4807 58F7 6C76
Re: [NEW] archivers/lrzip
how is it detecting how much memory to use? if it's looking at available physical memory without considering resource limits, could that be changed? there is precedent for programs to adjust RLIMIT_DATA from the default to the max available within the current limit if they know they're likely to need it - see e.g. src/usr.bin/sort/sort.c. if it's possible to automate this within reasonable bounds, this would be better than asking the user to adjust login.conf themselves. On 2022/05/20 19:35, Omar Polo wrote: > Hello Rubén, > > Rubén Llorente wrote: > > Port for lrzip included as an attachment. > > > > > > On Wed, May 18, 2022 at 12:49:51AM +0100, Stuart Henderson wrote: > > > please send ports as attachments to the list > > > > > Some comments on the port: > > - COMMENT should start with a lowercase. (maybe we can also shorten it > to 'compression utility for large files'?) > > - no need fo $V if it's used only in one place > > - upstream seems to have moved to github and has release other versions > (the latest one being 0.651.) Other popular package repositories are > using that version, so I think we should follow. patch-lrzip_private_h > needs to be regen'd. Using the autogenerated tarball from github > means that we need to run autoreconf & friends too. > > - SEPARATE_BUILD fails with latest version, so drop it > > - shuffle a bit to be in order with Makefile.template > > - targets are indented with a single tab > > - drop NO_TEST: it's used only when `make test' fails because there > isn't a regress suite. Here there isn't a regress suite but `make > test' doesn't fails. Avoiding to set NO_TEST helps when upstream then > adds one. > > - pkg/SECURITY it a new one for me. portcheck complains that's an extra > file. If the content is important, maybe add some comments in the > Makefile, but I'd drop it. > > - pkg/PLIST needed a regen. are you by any chance not using -current? > ports development should be done in -current. (asking because your > plist lacked a @static-lib marker, it's been there for a few years at > least...) > > - similarly, I'd drop pkg/MESSAGE. There's a little point in having two > files with the same content (pkg/MESSAGE and README), and READMEs are > handier. > > - nitpick: I'd reformat the DESCR at 72 chars, it reads better IMHO. > > - regarding the readme: > > : Suggested values follow: > : > : :datasize-cur=4096: \ > : :datasize-max=infinity: \ > : :stacksizemax=80M: > > hum... `infinity' it's hardly a "suggested" value. also, > stacksizemax? maybe just point out that this is a memory-hungry port > and that the default values may not be enough? > > > The good news is that with the above changes the ports looks good IMHO. > The bad news is that now the builds fails (link error in the embedded > lzma copy) so it needs more work (using lzma from ports if possible) > > I'm attaching a tarball with above suggestions included. > > Cheers, > > Omar Polo >
Re: [NEW] archivers/lrzip
Hello Rubén, Rubén Llorente wrote: > Port for lrzip included as an attachment. > > > On Wed, May 18, 2022 at 12:49:51AM +0100, Stuart Henderson wrote: > > please send ports as attachments to the list > > Some comments on the port: - COMMENT should start with a lowercase. (maybe we can also shorten it to 'compression utility for large files'?) - no need fo $V if it's used only in one place - upstream seems to have moved to github and has release other versions (the latest one being 0.651.) Other popular package repositories are using that version, so I think we should follow. patch-lrzip_private_h needs to be regen'd. Using the autogenerated tarball from github means that we need to run autoreconf & friends too. - SEPARATE_BUILD fails with latest version, so drop it - shuffle a bit to be in order with Makefile.template - targets are indented with a single tab - drop NO_TEST: it's used only when `make test' fails because there isn't a regress suite. Here there isn't a regress suite but `make test' doesn't fails. Avoiding to set NO_TEST helps when upstream then adds one. - pkg/SECURITY it a new one for me. portcheck complains that's an extra file. If the content is important, maybe add some comments in the Makefile, but I'd drop it. - pkg/PLIST needed a regen. are you by any chance not using -current? ports development should be done in -current. (asking because your plist lacked a @static-lib marker, it's been there for a few years at least...) - similarly, I'd drop pkg/MESSAGE. There's a little point in having two files with the same content (pkg/MESSAGE and README), and READMEs are handier. - nitpick: I'd reformat the DESCR at 72 chars, it reads better IMHO. - regarding the readme: : Suggested values follow: : : :datasize-cur=4096: \ : :datasize-max=infinity: \ : :stacksizemax=80M: hum... `infinity' it's hardly a "suggested" value. also, stacksizemax? maybe just point out that this is a memory-hungry port and that the default values may not be enough? The good news is that with the above changes the ports looks good IMHO. The bad news is that now the builds fails (link error in the embedded lzma copy) so it needs more work (using lzma from ports if possible) I'm attaching a tarball with above suggestions included. Cheers, Omar Polo lrzip.tar.gz Description: GNU Zip compressed data
