Bug 229773 - sysutils/dupd: update to version 1.7
Summary: sysutils/dupd: update to version 1.7
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Steve Wills
URL:
Keywords: needs-patch, needs-qa
Depends on:
Blocks:
 
Reported: 2018-07-14 17:55 UTC by jyri
Modified: 2019-01-15 14:46 UTC (History)
2 users (show)

See Also:
tom: maintainer-feedback+


Attachments
Preliminary port patch (3.88 KB, patch)
2018-08-04 01:46 UTC, Thomas Hurst
no flags Details | Diff
Proprosed Update/Changes (5.40 KB, patch)
2018-09-09 22:11 UTC, Nathan
tom: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jyri 2018-07-14 17:55:25 UTC
Track updating dupd to 1.7.

Please note that as of today this is preliminary, dupd 1.7 is not yet released.

In the previous update to 1.6 (226841) a handful of issues were discovered that made the port more work than it should be. But since the upstream version was already released, there was no way to change those (other than local patches here but that's more work).

The upstream (me) is pretty close to releasing dupd 1.7, so I'm filing this early in case it is possible to pre-test the build in the context of the FreeBSD port. I believe I've addressed all the changes that came up in the 1.6 port, but it'd be helpful to be sure. That way if any other changes are needed to make the port easy, I can integrate them upstream before calling 1.7 done.

In any case, I'll update this bug once the 1.7 release is done.
Comment 1 Thomas Hurst 2018-07-19 01:07:09 UTC
Thanks for this, rare to find developers so proactive :)

In the Makefile:

    INC+=-I/usr/local/include
    LIB+=-L/usr/local/lib

/usr/local is better spelled $PREFIX, with /usr/local just as a default if it isn't specified.

Also:

    dupd: src/optgen.c src/optgen.h $(OBJS)
          $(CCC) $(CFLAGS) $(OPT) $(OBJS)

$(OPT) is already included in CCC, and helpfully comes before CFLAGS because of that allowing the ports framework to override it - just needs removing here too.

Looks good other than that - self test passes on x64, will give x86 a bash when I get time.  I should also investigate usermode qemu and see if it works under more exotic architectures...
Comment 2 jyri 2018-07-19 01:26:23 UTC
Does the build need PREFIX called specifically 'PREFIX', or anything works?

'PREFIX' is very generic, and this definition would be FreeBSD-specific, so I'd prefer to give it a more descriptive name.


Speaking of OPT, why did you comment it out in 1.6?
Without -O3, the generated code is much, much slower (at least on Linux).
Comment 3 Thomas Hurst 2018-07-19 16:20:28 UTC
PREFIX is a pretty standard way of defining where your installed software lives, it's provided automatically by the ports framework and other similar systems.  It's easy enough to pass in as another name if you really want to use something else, but IMO that would be similar to using your own special names for CFLAGS and CC.

Removing hardcoded optimisation's standard porting behaviour: https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/dads-cflags.html

If it does make such a difference I'll add it back under the OPTIMIZED_CFLAGS option so users can choose to be more conservative if they prefer.
Comment 4 jyri 2018-07-20 06:08:55 UTC
In
https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/dads-cflags.html
it says "The system CFLAGS contains system-wide optimization flags."

So maybe -O3 is already there and inherited by the ports build? If so, nothing to do.

But if it isn't, then yes, make sure it is built with -O3 as it can make a huge difference. Every now and then I scare myself thinking I must've introduced a ~2x performance regression when I accidentally test a non-optimized build.
Comment 5 jyri 2018-07-20 06:23:12 UTC
On PREFIX, not sure if there is a standard (or at least de facto) on usage. If you're aware, let me know. I'd like to follow standard/common (across all platforms) behavior if at all possible.

My impression is that it's commonly used to specify where the built bits will go, but that's not the usage here. Here it is a root for where other libraries can be found. On that FreeBSD is different (from all other platforms I build on) because the libraries (e.g. sqlite3) are not on the default library path (/usr/lib) so this additional include/lib path needs to be added.

I guess I just talked myself into calling them INC_PREFIX and LIB_PREFIX or along those lines. They'll still need defaults in FreeBSD so the build works outside the ports framework.
Comment 6 Thomas Hurst 2018-07-21 23:19:31 UTC
-O2 is default, O3's quite aggressive and can break stuff, hence limiting it to specific ports which benefit from it by default, and letting more conservative users opt out.

Some quick testing with a mix of stuff shows quite modest differences between O2 and O3, between less than nothing and 15%, and it's reflected more by consumed CPU time than actual runtime.  Maybe it's more pronounced on newer systems (this is an old Westmere with wonky memory) or on gcc?

(I did notice it eating ~6GB on my CVS tree with a million small files, contrasting with jdupes that peaked at about 220MB - seems a bit excessive?)

Regarding PREFIX, fair point.  You are doing INC+= and LIB+=, so I can just pass those in myself and it won't blat them, so you can probably just leave that.
Comment 7 jyri 2018-07-22 08:28:41 UTC
Will the port build pass in any prefix other than /usr/local in practice?

I can make it take a var (e.g. LIB_PREFIX) and if it set use it as-is, or if not set, default it to /usr/local

OTOH, while I'm all for making it flexible I also don't want to add clutter that nobody will use. So if the port build system will ultimately just always pass in /usr/local as the prefix, might as well leave it as is. But if this can vary in FreeBSD installations, then it's worth making it configurable.


I haven't done any meaningful testing between -O2 vs. -O3 so can't really say. Perhaps -O2 is fine. I also have not done any performance testing on FreeBSD. But if you run into any performance weirdness on FreeBSD please file a bug, I'll check it out. I primarily test on Linux (w/gcc) and secondarily on Solaris (w/Sun cc).


dupd attitude is that RAM is there to be used, so it doesn't limit memory usage unless you ask (--buflimit). If you want, send me output of 'dupd scan -v' and I'll play around with a similar file set in case there is any unwarranted inefficiency there.
Comment 8 jyri 2018-07-25 04:04:07 UTC
I added the option of using INCLUDE_DIR and LIB_DIR to change /usr/local/ to something else if needed.

I've tested on
FreeBSD 11.1-RELEASE i386
FreeBSD 11.1-RELEASE amd64

If time allows I'll try to do a 1.7 release next weekend. If there's anything else to include let me know before then, if possible.
Comment 9 Thomas Hurst 2018-08-04 01:45:51 UTC
Sorry for the break there, awkward combination of busyness, slackness, and forgetfulness.  Mostly the latter two.

Yeah, users can set LOCALBASE to move PREFIX to somewhere else; I ran with it set to /usr/pkg for a while many years ago.  It broke quite a bit, but it *should* work and resulted in a lot of PRs :)

Could probably achieve basically the same as your latest changes just by setting INC?= and LIB?=, but it amounts to more or less the same thing from my end - I just set the variables and your Makefile Just Works[tm].

Could still do with OPT being similarly set optionally, just so that's easily overridden?  Would let users `make OPT=-O3 -fomit-frame-pointer -fomg-optimize`, and in ports case set to the empty string or unset with OPTIMIZED_CFLAGS.  I can still override it by setting CFLAGS but it ends up with a rather ugly "-O3 -O2 -O3" dance as the various flags fight.

Also, make test passes on my x86 jail \o/

Will attach a preliminary patch for the port.
Comment 10 Thomas Hurst 2018-08-04 01:46:53 UTC
Created attachment 195833 [details]
Preliminary port patch
Comment 11 jyri 2018-08-07 01:35:31 UTC
I ended up busy previous weekend and won't have time in the next few, so won't do a 1.7 release until late August or so. I'll look into doing the above tweaks then.
Comment 12 jyri 2018-09-08 06:52:15 UTC
I now set OPT via ?= so you can override it more cleanly as noted above ("gmake OPT=-O2" or whichever flags preferred).

I released dupd-1.7 today, so it's ready to be picked up.
Comment 13 Nathan 2018-09-09 21:15:21 UTC
I wouldn't have OPTIMIZED_CFLAGS as default. FreeBSD uses -O2 as default
Also 1.7 is out
Shouldn't need GH_TAGNAME unless you are needing a certain commit for a bug or etc.


Why use manual install when the source's Makefile works fine?
Comment 14 Nathan 2018-09-09 22:11:37 UTC
Created attachment 196992 [details]
Proprosed Update/Changes

 sysutils/dupd:
 
 * Update to 1.7
 * Rearange variables
  -- To match correct ordering
 * Add DOCS to options
 * Strip binary
 
 Changelog: https://github.com/jvirkki/dupd/blob/1.7/ChangeLog

Built fine on:
poudriere(amd64/i386): 12-current, 11.2, 10.4
poudriere(mips,mips64,armv6,arm64)
Comment 15 Thomas Hurst 2018-09-28 15:12:51 UTC
Comment on attachment 196992 [details]
Proprosed Update/Changes

Looks good, test suite happy on amd64 and i386, poudriere happy across a range of jails.

Thanks!
Comment 16 commit-hook freebsd_committer 2019-01-15 14:44:37 UTC
A commit references this bug:

Author: swills
Date: Tue Jan 15 14:44:26 UTC 2019
New revision: 490381
URL: https://svnweb.freebsd.org/changeset/ports/490381

Log:
  sysutils/dupd: update to version 1.7

  PR:		229773
  Submitted by:	jyri@virkki.com (original patch)
  Submitted by:	Nathan <ndowens@yahoo.com> (final patch)
  Approved by:	jyri@virkki.com (maintainer)

Changes:
  head/sysutils/dupd/Makefile
  head/sysutils/dupd/distinfo
  head/sysutils/dupd/files/
  head/sysutils/dupd/pkg-plist
Comment 17 Steve Wills freebsd_committer 2019-01-15 14:45:31 UTC
Committed, thanks!
Comment 18 Steve Wills freebsd_committer 2019-01-15 14:46:30 UTC
Committed. I accidentally referenced the wrong user/email in the commit as the maintainer, sorry about that and the fact that this took so long to commit. Thanks for the patch and approval.