On 2025/02/28 18:11, Jeremie Courreges-Anglas wrote:
> On Thu, Feb 27, 2025 at 08:07:40PM +0100, Volker Schlecht wrote:
> > On 2025-02-27 18:43, Stuart Henderson wrote:
> > > two things definitely need changing:
> > >
> > > - two symbols were removed; -DSQLITE_ENABLE_RTREE brings them back
> >
> > Right, so does --enable-rtree - would that be ok as well?
> >
> > > please add a BUILD_DEPENDS on lang/jimtcl so that the build environment
>
> Erm, lang/jimtcl itself depends on databases/sqlite3 so it can't be
> available in bulk builds started on clean machines, and we can't add
> a build dep on it either.
Ah dammit. And I'd forgotten that it BDEPs on lang/tcl so actually it'd
be a worse choice than tcl itself.
> I'd rather patch out the hidden dep so that the build is
> deterministic, even on a machine where jimtcl is already installed.
Yes, that approach makes sense.
> > done
> >
> > > personally I'd do these the other way round and get rid of ${VERSION} :
> > >
> > > rm ${PREFIX}/lib/libsqlite3.so{,.0}
> > > mv ${PREFIX}/lib/libsqlite3.so.*
> > > ${PREFIX}/lib/libsqlite3.so.${LIBsqlite3_VERSION}
>
> I usually find this kind of construct suspicious, but indeed soname is
> correctly passed and the resulting libsqlite.so.X.Y looks sane.
I thought that too and checked, but didn't think to point it out.
Maybe it's worth a small comment. At least that way, somebody seeing
it might then understand that they shouldn't blindly copy it for
another port.
> > -# for x11/gnome/tracker
> > +
> > +CONFIGURE_ARGS += --enable-rtree
> > +
> > +# originally for x11/gnome/tracker
> > CONFIGURE_ARGS += --enable-fts5
> >
> > # for mozilla
> > CFLAGS+= -DSQLITE_ENABLE_UNLOCK_NOTIFY \
> > - -DSQLITE_ENABLE_FTS3 \
> > -DSQLITE_ENABLE_DBSTAT_VTAB \
> > - -DSQLITE_ENABLE_COLUMN_METADATA=1 \
> > - -DSQLITE_ENABLE_SESSION \
> > - -DSQLITE_ENABLE_PREUPDATE_HOOK
> > + -DSQLITE_ENABLE_COLUMN_METADATA
> > +
> > +CONFIGURE_ARGS += --enable-fts4 \
> > + --enable-fts3
> > +
> > +# for deno
> > +CONFIGURE_ARGS += --enable-session
>
> I know you're not responsible for the current situation, but IMHO
> those "for this" "andinitially for that" comments don't bring much
> value . One can look at cvs blame/log to understand why we enabled an
> option. I don't feel too strongly about it but the Makefile would be
> more readable if we merged CONFIGURE_ARGS in a single block.
That's a fair point, also things change over time and we don't really
know what might now which ports might want certain options to be enabled
any more.
I'm not a fan of this mixture of CFLAGS and CONFIGURE_ARGS to enable
things, but what can we do..
> > +BUILD_DEPENDS = converters/sqlite2mdoc \
> > + lang/jimtcl
> > +
> > +do-configure:
> > + cd ${WRKSRC} && \
> > + ${CONFIGURE_ENV} ./configure ${CONFIGURE_ARGS}
>
> This lacks ${SETENV}, but instead of creating your own do-configure
> I'd prefer that you use CONFIGURE_STYLE=simple which seems to work
> just as fine.
ack, +1
