Re: Proposed changes to wesnoth-1.16

On Thu, Jan 11, 2024 at 7:25 AM P. J. McDermott <[email protected]> wrote:

> > > And like I said, note that 1.18 will also have src/modules/lua/ without
> > > local changes, so it can finally be replaced with the system copy of Lua
> > > 5.4.  Repacking is generally recommended to make sure the system copy is
> > > used and thus there's no chance the embedded copy is accidentally used,
> > > hiding from efforts to fix a security vulnerability across all copies in
> > > the Debian archive (or worse, in Ubuntu where it will more likely
> > > never get fixed).  But I guess you prefer not to repack to remove ECCs
> > > (whether jQuery+tablesorter or Lua)?
> >
> > If you don't trust the build system used by the package and want to
> > ensure system build-deps are used, you can simply rm -rf the embedded
> > code copies in the package's clean target (dpkg-source ignores removed
> > files so that's generally safe to do). I think this is a lesser evil
> > vs repacking the upstream source tarball, and I'm not convinced it's
> > appropriate or necessary to do so for wesnoth, sorry.
>
> dpkg-source will ignore but also complain in the build log about every
> single removed file, but otherwise sure, that works.  To be clear,
> you're not convinced repacking is necessary, but you'd be OK with
> removing the Lua ECC in the clean target?  Or you're not convinced
> either is necessary?
>
> I certainly don't trust the build system, because there's currently
> nothing *to* trust.  Wesnoth 1.17/1.18's build systems (CMake and SCons)
> don't even have an option to use a system copy of Lua 5.4.

Yep, please go ahead and remove embedded code copies with d/rules
clean. This is also what I do with other packages that I maintain
where upstream's build system doesn't give me the flexibility of not
using embedded code copies, e.g. supertuxkart [1].

[1] https://sources.debian.org/src/supertuxkart/1.4%2Bdfsg-3/debian/rules/

> Upstream wants Lua compiled as C++ [1][2] so that it can recover from
> C++ exceptions [3][4].  Lua's build system doesn't compile as C++,
> but Debian does [5].  As does Arch [6] but not Fedora [7].  Wesnoth also
> still wants some compile-time definitions [8], although the LUA_COMPAT_*
> ones are from Lua 5.3 and are unused in 5.4, so that leaves the Windows
> strcoll() redefinition and exception rethrowing.
>
> So since Lua compiled as C++ isn't (yet) universal among distributions
> or Lua itself, and Wesnoth still wants some compile-time changes,
> upstream might not be willing to support using system Lua (even though
> they did before 1.9.0), especially this late in their development cycle
> while they're understandably focused on getting a release ready.  But
> I've developed and submitted [9] a patch series to add support to both
> build systems for using system Lua 5.4 C++ on non-Windows platforms.
>
> In the meantime, actually the only way to use system Lua 5.4 C++ without
> a large patch series is to remove the ECC, replace it with header
> symlinks, and hack around the build system:
>
> debian/clean:
> +src/modules/lua/
>
> debian/rules:
> +include /usr/share/dpkg/buildtools.mk
>  override_dh_auto_configure:
> +       1>source_lists/lua
> +       mkdir -p src/modules/lua/.git/
> +       for f in /usr/include/lua5.4/*.h; do \
> +               ln -sf "$${f}" "src/modules/lua/$${f##*/}"; \
> +       done
>         dh_auto_configure -- $(CMAKE_SWITCHES)
> +       sed -i "s|\$$| $$($(PKG_CONFIG) --libs lua5.4-c++)|" \
> +               build/src/CMakeFiles/wesnoth.dir/link.txt
>
> (I don't like having to modify CMake's link command with sed, but CMake
> apparently doesn't have anything like Automake's LIBS, and of course
> LDFLAGS would put -l flags in the wrong place.)
>
> [1]: https://github.com/wesnoth/wesnoth/blob/9315079/src/CMakeLists.txt#L90
> [2]: https://github.com/wesnoth/wesnoth/blob/ac259f0/src/SConscript#L124
> [3]: https://github.com/wesnoth/wesnoth/commit/e4d0ae0
> [4]: https://github.com/lua/lua/blob/9be74cc/ldo.c#L47
> [5]: https://salsa.debian.org/lua-team/lua5.2/-/commit/fa2dc77c
> [6]: https://gitlab.archlinux.org/archlinux/packaging/packages/lua/-/commit/4e97e19d
> [7]: https://src.fedoraproject.org/rpms/lua/blob/rawhide/f/lua.spec
> [8]: https://github.com/wesnoth/wesnoth/blob/c44cc27/src/wesnoth_lua_config.h
> [9]: https://github.com/wesnoth/wesnoth/pull/8234

I'm happy to defer to what you think is best here - whichever approach
you think is more maintainable, whether it's to hack around the build
system with d/rules or add the patch series that you linked above (I
guess that kind of depends on how likely you think the patch series
would be accepted upstream - I wouldn't really want to carry it around
indefinitely).

> > We could also just drop src:wesnoth-1.16 entirely at this point (we
> > can just file an RM request to have it be removed from sid, and if
> > it's done before Debian Import Freeze then it'll automatically get
> > removed from Ubuntu Noble as well).
>
> I don't think we should RM src:wesnoth-1.16 until after noble's Debian
> Import Freeze (and better yet, after the release of 1.16.12).  After
> discussion with upstream [10], I personally think the best option
> overall is to have both versions in noble.
>
> As you know, whatever version is in Ubuntu at the Debian Import
> Freeze remains through the release's lifetime (except for the PPA you
> maintain), since I think no one on this team is an Ubuntu MOTU team
> member who could maintain this team's packages in Ubuntu universe
> (someone please correct me if I'm wrong).
>
> So if severe bugs are found in 1.17.26, 1.16.11 could be a less buggy
> fallback option for Ubuntu users for at least single-player games (the
> multiplayer server for 1.16 will clear out pretty soon after the 1.18.0
> release).
>
> Either way, of course we shouldn't RM src:wesnoth-1.16 before
> src:wesnoth-1.18 leaves NEW, because then we run the risk of the latter
> taking too long in NEW and noble having no wesnoth packages at all.
>
> [10]: https://github.com/wesnoth/wesnoth/issues/8187

That's fine, I just wasn't sure earlier whether you deliberately
wanted both wesnoth 1.16 and 1.18 to be made available in Noble.

> > The plan sounds fine to me, thanks for volunteering to tackle this!
> > I'm happy to sponsor/upload wesnoth when it's ready, feel free to ping
> > me and I'll try to get back ASAP.
>
> Thanks for sanity checking the plan and for the sponsorship offer!  When
> I have things ready to upload, should I send mail To: you and Cc: the
> list or just send To: the list?  I'm asking what's more convenient for
> you, in case you filter and don't always read list mail but do read mail
> sent To: you.

Mail sent directly to me will get my attention faster, but I'll try to
be better about responding to mail both on and off list.

> And, do you have any thoughts on the campaigns dependency/AppStream
> bug [11]?  The issue is that wesnoth-1.16-core provides the executable
> and therefore also the .desktop and .appdata.xml files, but since
> the campaigns are split out into individual packages and only the
> wesnoth-1.16 metapackage depends on them, users only get the executable
> without any campaigns when installing through "software center" package
> manager frontends that find application packages via AppStream data.
>
> Rhonda suggested some kind of manual AppStream data adjustments, but I
> can't find anything in the AppStream specification about adding package
> dependencies or anything else relevant, so I don't know what she means.
>
> Would you object to making wesnoth-*-core recommend all the campaign
> packages?  I found a package in Debian with AppStream metadata and
> Recommends and tested that gnome-software with default APT configuration
> does indeed install the Recommends packages.  Or do you have any other
> suggestions?
>
> [11]: https://bugs.launchpad.net/ubuntu/+source/gnome-software/+bug/2033222

I don't really have any context into how appstream or Ubuntu software
center works, again happy to defer to what you or Rhonda think is best
here. I'm fine with having -core recommend all the campaign packages.

Regards,
Vincent