Why not vendor pxupgma and pxnj?

[nileshpatra]

Hi,

The compilation commands for pxupgma and pxnj are given in src/Makefile.in, however these binaries are not built on the normal build. Is there a specific reason for doing that? If not, please consider applying the patch:

Description: Also build and vendor pxupgma pxnj
Author: Nilesh Patra <nilesh@debian.org>
Last-Update: 2021-07-10
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -357,7 +357,7 @@
 %.o: ./%.cpp ./%.h
    $(CXX) $(CXXFLAGS) $(OPT_FLAGS) $(CPPFLAGS) $(LDFLAGS) -c -fmessage-length=0 -MMD -MP -MF "$(@:%.o=%.d)" -o "$@" "$<"

-PROGS := pxaa2cdn pxbdsim pxboot pxbp pxcat pxclsq pxcltr pxcolt pxcomp pxconsq pxfqfilt pxlog pxlssq pxlstr pxmono pxmrca pxmrcacut pxmrcaname pxnw pxpoly pxrecode pxrevcomp pxrls pxrlt pxrmk pxrms pxrmt pxrr pxs2fa pxs2nex pxs2phy pxseqgen pxssort pxsstat pxsw pxt2new pxt2nex pxtcol pxtcomb pxtgen pxtlate pxtrt pxtscale pxvcf2fa $(NLOPT_PROGRAMS)
+PROGS := pxaa2cdn pxbdsim pxboot pxbp pxcat pxclsq pxcltr pxcolt pxcomp pxconsq pxfqfilt pxlog pxlssq pxlstr pxmono pxmrca pxmrcacut pxmrcaname pxnw pxpoly pxrecode pxrevcomp pxrls pxupgma pxrlt pxnj pxrmk pxrms pxrmt pxrr pxs2fa pxs2nex pxs2phy pxseqgen pxssort pxsstat pxsw pxt2new pxt2nex pxtcol pxtcomb pxtgen pxtlate pxtrt pxtscale pxvcf2fa $(NLOPT_PROGRAMS)

 # default all target
 all: $(PROGS)

[nileshpatra]

On a related note, we have more patches here IMO, they are appropriate for the upstream as well. If deemed appropriate, please consider to apply.

[josephwb]

Those programs were purposefully removed from the default build because they are meant more for instruction than publication-worthy analysis. These may be removed entirely.

I am looking at your other patches now.

The addition of $(CXXFLAGS) in the Makefile.in doesn't seem to do anything since it is not defined anywhere. I suppose we should enable this to be passed from configure..

Why the addition of -lnlopt_cxx? Is -lnlopt not sufficient? It is all we have ever had.

[nileshpatra]

On Sat, 10 Jul 2021 at 21:59, Joseph W. Brown @.***> wrote:

Those programs were purposefully removed from the default build http://fd445c32b9b77d47a5153e7111341f5b4a88b8a5 because they are meant more for instruction than publication-worthy analysis. These may be removed entirely.

Fair.

I am looking at your other patches now.

The addition of $(CXXFLAGS) in the Makefile.in doesn't seem to do anything since it is not defined anywhere.

Yeah, that's passed by debhelper, and specific to debian and set while building the value of it is this:

$ dpkg-buildflags --get CXXFLAGS -g -O2 -fdebug-prefix-map=/home/nilesh=. -fstack-protector-strong -Wformat -Werror=format-security

Here the -fdebug-prefix-map is for reproducible builds. It does not add a lot of utility for you, but it does for us (as downstream) If you could do so, that would be great.

I suppose we should enable this to be passed from configure..

That'd be fine as well.

Why the addition of -lnlopt_cxx? Is -lnlopt not sufficient? It is all we have ever had.

It ended with errors in older versions of phyx, and the patch is from then. It works fine now, I'll probably drop this, thanks!

[josephwb]

Ok I will get to this later today. Thanks for the help.

[josephwb]

Hey @nileshpatra. I did not draft the original makefile. It seems to me that the OPT_FLAGS we use is standing in for CXXFLAGS. Would it work for you if I renamed the former to the latter? That would mean that any CXXFLAGS you pass will be appended to the ones we typically have.

[nileshpatra]

It likely would, consider doing so please :)

[josephwb]

I'm having trouble trouble merging our hardcoded flags with those passed in with configure, so for the moment CXXFLAGS is its own thing as you had originally suggested (as of f655915). Please let me know if this works. I will clean things up later.

[nileshpatra]

I will test tomorrow and let you know