[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Updating Minetest to 5.5



Simon McVittie <smcv@debian.org> writes:

> On Fri, 25 Feb 2022 at 07:29:48 +0100, Markus Koschany wrote:
>> You probably don't need a new source
>> package just for the irrlicht fork if minetest is the only "consumer"
>
> Expanding on that, I have some questions I want to ask:

I contributed to Minetest a bit about 10 years ago and drew the Minetest
icon that is still used today. I am not part of the current development
team – I would like to, but the only way towards that is implementing
features and I am much better at finding bugs and doing code review.
(Also I think it is a bad rule, as it leads to lack of reviewers.)

> * Is any other package in Debian likely to depend on Minetest's fork
>   of irrlicht?

Extremely unlikely. The fork is mostly deleting about everything
Minetest does not use, plus changing & adding some minor things.
There exists a single commit that deletes about 200k lines and
it was approved in a day without review, causing regressions.
Those have been fixed by now, but I want to be clear here …

The current state of things is that IrrlichtMT is basically irrlicht
minus those 200k or so lines, plus some other stuff. The stated plan
seems to be to eventually get rid of Irrlicht completely – as enough
of the core developers of Minetest believe that it is a good idea to
fold all of that back into Minetest. The Minetest coredevs also want
to get rid of the fixed pipeline long-term, essentially removing the
ability to run it on lower-spec devices, because shaders look shiny.

I had been told that in the future a baseline would be OpenGL ES 2.0
at minimum, but when shaders did not work on the (OGLES2-conformant)
Mali 400 GPU one of the developers said they would not put in work …

The history behind all that is quite complicated and I am not an expert
on the matter, but basically Minetest is not using many facilities that
Irrlicht offers. Many of the Minetest core developers will concede that
e.g. Irrlicht particles are much more performant than what Minetest did
bolt on to it (in terms of drawcalls) … or that Irrlicht can do shadows
that look better than what Minetest draws with vastly less requirements
(in terms of needing modern GPU hardware), but ultimately, those things
were not considered when the Irrlicht particle implementation & shadows
implementation were deemed to be “superfluous” and removed in the great
IrrlichtMT code “cleanup” (that 200k line deletion commit I mentioned).

> * Does Minetest's fork of irrlicht have a stable API?

Minetest core developers do not believe in stable APIs – except if they
themselves have both documented the API outside of code comments and it
turns out that what they documented is actually used … for example, the
200k line cleanup removed support for several graphics formats, without
a check before if maybe some Minetest mods or texture packs are relying
on some of those. Not only had mod developers warned about removing TGA
support, it was added back only after I pointed out what it broke. I am
all for removing stuff that represents attack surface, but I doubt that
the IrrlichtMT transition has been properly thought trough after what I
have seen. If you doubt that, just ask for stable APIs in #minetest-dev
@ libera.chat … or take a look at the Minetest issue tracker on GitHub.

To be clear, IrrlichtMT not having a stable API is negatively affecting
maintenance work on Minetest itself, since it makes it difficult to use
git bisect, as Minetest core developers reject having it as a submodule
for Minetest proper. You basically must upgrade everything in lockstep.
Also, the history of IrrlichtMT contains only a very recent part of the
Irrlicht history, making it next to impossible to debug some mean bugs.
Then again, the developers do not seem to care too much about that bit.

> If the answer to either of those is no, and especially if the
> answer to *both* is no (as I suspect), then I think packaging
> Minetest's fork of irrlicht as part of the minetest source package (a
> vendored/bundled/embedded copy), and most likely linking it into minetest
> statically, would actually be *better* than packaging it separately. If
> Minetest upstream release them together, then just don't remove it;
> if Minetest upstream release them separately, dpkg source format 3.0
> (quilt) can deal with combining multiple upstream tarballs into one
> source package.

I think the release of IrrlichtMT is basically just a git tag on the
IrrlichtMT repository. I bet GitHub offers tarballs for those somehow.

> When we package libraries as separate packages, we do that in order to
> get the benefits of being able to share them between multiple library
> users, but there is a packaging and coordination cost in having more
> source packages to deal with.
>
> If you're packaging minetest and irrlicht-minetest as separate source
> packages, then I'm concerned that you'll be paying the cost of a
> separately-packaged library, without actually getting any of the benefits!

I totally agree here. Especially in light of the plan to fold everything
into Minetest eventually (which I doubt will be done anytime soon, but I
guess it will be done at some point) it seems entirely appropriate here.

> Obviously it would be preferable if minetest could use the packaged
> version of upstream irrlicht, but if that's not an option because their
> irrlicht is not compatible, then proliferating packages is not actually
> helping us. If we compare the possible setups with bundled and separate
> irrlicht:
>
> * bundled:
>     src:irrlicht
>     src:minetest (contains patched irrlicht)
>     src:supertuxkart (contains patched irrlicht)
> * separate:
>     src:irrlicht
>     src:irrlicht-minetest (is a patched irrlicht)
>     src:irrlicht-supertuxkart (is a patched irrlicht)
>     src:minetest (requires irrlicht-minetest)
>     src:supertuxkart (requires irrlicht-supertuxkart)
>
> then you'll notice that in each case, fixing a security vulnerability in
> irrlicht would require three uploads - so packaging each fork separately
> would not actually reduce the security team's work at all.

I suspect that maybe only a few patches would be needed to make Minetest
compile with upstream Irrlicht, since the incompatibilities come almost
entirely from the “delete code that we are not using” described above.

However … someone would have to put in the effort. I can try to help
figuring out if it is feasible and help sorting patches, but I can
not do it alone. If anyone is interested in this, just email me.

A side effect would be to figure out what can be upstreamed,
as IrrlichtMT actually does contain fixes for Irrlicht bugs.
It also has some new bugs, but let's not go there right now.

> The yquake2 source package is an example of combining multiple upstream
> tarballs, if you need one (upstream does separate releases of the game
> engine and the CTF plugin, but the proprietary patch data managed by
> game-data-packager includes the CTF game-mode, so being able to install
> the engine without the CTF plugin seemed like it would just create a
> broken situation for no real benefit).
>
>     smcv

Attachment: signature.asc
Description: PGP signature


Reply to: