Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "MINETEST_MOD_PATH" environment variable #11515

Merged
merged 4 commits into from
Oct 6, 2021

Conversation

emixa-d
Copy link
Contributor

@emixa-d emixa-d commented Aug 5, 2021

This patch adds an environment variable "MINETEST_MOD_PATH". It functions like "MINETEST_SUBGAME_PATH", but for individual mods. This is necessary for installing minetest mods with Guix (see https://issues.guix.gnu.org/49828), otherwise Minetest won't find the mods.

I tested that installing mods from ContentDB with the built-in installer still works, even if "MINETEST_MOD_PATH" is set.

To do

This PR is Ready for Review.

How to test

Set "MINETEST_MOD_PATH" to "/location/of/some/mod:/location/of/some/other/mod", start Minetest, create a world, select the two mods, start the game, verify the two mods are actually loaded. Test that mods can still be installed with the built-in installer, and they are installed in ~/.minetest/mods, not /location/of/some/mod or /location/of/some/other/mod.

@emixa-d emixa-d changed the title Minetest mod path Add "MINETEST_MOD_PATH" environment variable Aug 5, 2021
@emixa-d
Copy link
Contributor Author

emixa-d commented Aug 5, 2021

The patch is rebased on 'master' now.

@emixa-d emixa-d marked this pull request as ready for review August 5, 2021 12:03
@sfan5 sfan5 added @ Server / Client / Env. Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand labels Aug 5, 2021
@emixa-d
Copy link
Contributor Author

emixa-d commented Sep 9, 2021

It's rebased on latest master (untested).

@sfan5 sfan5 removed the Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand label Sep 13, 2021
@sfan5
Copy link
Member

sfan5 commented Sep 13, 2021

Well I guess if we already have MINETEST_SUBGAME_PATH this is fine too.

@sfan5 sfan5 mentioned this pull request Sep 13, 2021
builtin/mainmenu/pkgmgr.lua Outdated Show resolved Hide resolved
src/content/subgames.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_mainmenu.cpp Outdated Show resolved Hide resolved
@emixa-d
Copy link
Contributor Author

emixa-d commented Sep 14, 2021

I'll test the new version and try to write some tests.

Edit: I added a unit test.

Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The env variable should be documented somewhere.

src/content/subgames.cpp Show resolved Hide resolved
src/content/subgames.cpp Outdated Show resolved Hide resolved
@emixa-d emixa-d force-pushed the minetest-mod-path branch 2 times, most recently from 3e25bab to 20d273a Compare September 14, 2021 17:49
@emixa-d
Copy link
Contributor Author

emixa-d commented Sep 14, 2021

@Desour: The similar environment variable MINETEST_SUBGAME_PATH is documented in doc/minetest.6. Would that be a good location to document MINETEST_MOD_PATH as well, or were you thinking of some other location?

This adds an environment variable MINETEST_MOD_PATH.
When it exists, Minetest will look there for mods
in addition to ~/.minetest/mods/.  Mods can still be
installed to ~/.minetest/mods/ with the built-in installer.

With thanks to Leo Prikler, sfan5 and DS for reviewing.
The client still needs to be tested manually.
@emixa-d
Copy link
Contributor Author

emixa-d commented Sep 14, 2021

Changed Desour->DS

@emixa-d
Copy link
Contributor Author

emixa-d commented Sep 14, 2021

I did a little testing on GNU Guix, which has a patched version of Minetest to support MINETEST_MOD_PATH and has a few mods packages, e.g. mesecons:

$ guix environment --ad-hoc minetest minetest-mesecons -- ./bin/minetest

Check list

  • can create and start a new world with guix' minetest, and use mesecons in that world
  • if I use the built-in installer to install ‘Advanced Trains’, and enable it,
    I can use both blocks from ‘Advanced Trains’ and mesecons

Even though I installed mesecons via guix, I can still install mesecons from the
built-in installer. However, this results in the ‘mesecons’ mods being displayed
twice in the ‘select mods’ menu, and when the world is started, this results in
‘ModError: Unresolved name conflicts for mods "mesecons_wires", ...’, which doesn't
seem unreasonable?

Now I try to resolve this conflict by deleting the mod from ContentDB via the built-in
(de-)installer, which succeeds! (Though I had to modify fs::RecursiveDelete because
/bin/rm doesn't exist on Guix System.)

@Desour
Copy link
Member

Desour commented Sep 14, 2021

@Desour: The similar environment variable MINETEST_SUBGAME_PATH is documented in doc/minetest.6. Would that be a good location to document MINETEST_MOD_PATH as well, or were you thinking of some other location?

Sounds fine, I guess.

@emixa-d
Copy link
Contributor Author

emixa-d commented Sep 15, 2021

The latest version should address everything I think.

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed some minor changes, hope you don't mind.
Looks good now

@emixa-d
Copy link
Contributor Author

emixa-d commented Sep 16, 2021

@sfan5: The changes seem fine to me!

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sure, why not?

@emixa-d
Copy link
Contributor Author

emixa-d commented Oct 6, 2021

I noticed one issue lately, when combining what is in Guix known as ‘profiles’ (Guix uses an early version of this MINETEST_MOD_PATH patch).

If I install minetest and a mod, say homedecor, e.g. with guix install minetest minetest-homedecor-modpack, then the MINETEST_MOD_PATH will be something like

MINETEST_MOD_PATH=$HOME/.guix-profile/share/minetest/mods

and this directory contains homedecor, basic_materials and unifieddyes, as expected. Suppose I want to test out another mod, say coloredwood, in a temporary environment (guix environment --ad-hoc minetest coloredwood), then MINETEST_MOD_PATH becomes

MINETEST_MOD_PATH=/gnu/store/[bla-bla]/share/minetest/mods:$HOME/.guix-profile/share/minetest/mods

where the former directory contains the coloredwood mod and its dependencies unifieddyes and basic_materials, and the latter contains homedecor, basic_materials and unifieddyes. This is all as expected, but it appears minetest doesn't like a mod appearing in multiple places, because if I start Minetest, I see mods appearing multiple times in the ‘select mods’ menu, and on standard error, I see ERROR[Main]: mod "this" has unsatisfied dependencies: "x" "y" messages.

@sfan5 sfan5 merged commit 9fab5d5 into minetest:master Oct 6, 2021
@rollerozxa
Copy link
Member

It looks like the environment variable doesn't work for loading mods that are split into a modpack, such as WorldEdit. If I have the Awards mod and WorldEdit mod installed into /usr/share/minetest/mods/ and launch Minetest like MINETEST_MOD_PATH="/usr/share/minetest/mods" minetest then the Awards mod will show up and can be enabled, whereas the WorldEdit mod does not show up.

@emixa-d
Copy link
Contributor Author

emixa-d commented Dec 11, 2021

WorldEdit seems to work fine in guix (using guix shell --pure minetest minetest-worldedit), though guix uses an older version of the patch IIRC.

In guix, MINETEST_MOD_PATH is set to $GUIX_ENVIRONMENT/share/minetest/mods. This directory contains a subdirectory Minetest-WorldEdit, which contains a file modpack.conf, worldedit,'worldedit_brush, ...

Now testing with multiple mods ...

@emixa-d
Copy link
Contributor Author

emixa-d commented Dec 11, 2021

worldedit + mesecons seems to work for me

@rollerozxa
Copy link
Member

Oh never mind, it works for me now, I don't know why it didn't work before... sorry 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants