Page MenuHomeWildfire Games
Differential D1142

Adds a .pyromod installation
ClosedPublic
Actions

Authored by Itms on Dec 11 2017, 9:35 PM.

Details

Reviewers
vladislavbelov
Commits
rP21726: Add a mod installer, fixes #4027.
Trac Tickets
#4027
Summary

Adds a support of .pyromod files (actually .zip files). Which can be installed by Pyrogenesis in the mod directory.

Test Plan
  1. Apply the patch and build the game
  2. Make/download mod-name.zip
  3. Install the mod pyrogenesis "mod-path"

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.Dec 11 2017, 9:35 PM
elexis awarded a token.Dec 11 2017, 9:41 PM
elexis added a subscriber: elexis.Dec 11 2017, 9:46 PM
elexis added inline comments.
source/ps/ModInstaller.cpp
90 ↗(On Diff #4722)

wrename?

vladislavbelov added inline comments.Dec 11 2017, 9:59 PM
source/ps/ModInstaller.cpp
90 ↗(On Diff #4722)

Sounds good, do we have something for copy?

Vulcan added a subscriber: Vulcan.Dec 11 2017, 10:07 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
../../../binaries/system/libengine.a(ModInstaller.o): In function `copy_file':
/usr/include/boost/filesystem/operations.hpp:381: undefined reference to `boost::filesystem::detail::copy_file(boost::filesystem::path const&, boost::filesystem::path const&, boost::filesystem::copy_option, boost::system::error_code*)'
collect2: error: ld returned 1 exit status
make[1]: *** [../../../binaries/system/pyrogenesis] Error 1
make: *** [pyrogenesis] Error 2
Build (debug)...
../../../binaries/system/libengine_dbg.a(ModInstaller.o): In function `boost::filesystem::copy_file(boost::filesystem::path const&, boost::filesystem::path const&, boost::filesystem::copy_option)':
/usr/include/boost/filesystem/operations.hpp:381: undefined reference to `boost::filesystem::detail::copy_file(boost::filesystem::path const&, boost::filesystem::path const&, boost::filesystem::copy_option, boost::system::error_code*)'
collect2: error: ld returned 1 exit status
make[1]: *** [../../../binaries/system/pyrogenesis_dbg] Error 1
make: *** [pyrogenesis] Error 2
Running release tests...
Running cxxtest tests (308 tests)............................................................................................................................................................................................................................
In TestComponentScripts::test_scripts:
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:50: Error: Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/components") / pathname)
ERROR: JavaScript error: simulation/components/tests/setup.js line 22
TypeError: "IID_RallyPoint" is read-only
  Engine.RegisterInterface@simulation/components/tests/setup.js:22:2
  @simulation/components/interfaces/RallyPoint.js:1:1
  @simulation/components/tests/test_Foundation.js:6:1
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Foundation.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:50: Error: Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/components") / pathname)
ERROR: JavaScript error: simulation/components/tests/setup.js line 22
TypeError: "IID_RallyPoint" is read-only
  Engine.RegisterInterface@simulation/components/tests/setup.js:22:2
  @simulation/components/interfaces/RallyPoint.js:1:1
  @simulation/components/tests/test_GarrisonHolder.js:13:1
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_GarrisonHolder.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:50: Error: Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/components") / pathname)
ERROR: JavaScript error: simulation/components/tests/setup.js line 22
TypeError: "IID_RallyPoint" is read-only
  Engine.RegisterInterface@simulation/components/tests/setup.js:22:2
  @simulation/components/interfaces/RallyPoint.js:1:1
  @simulation/components/tests/test_GuiInterface.js:25:1
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_GuiInterface.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests
Vulcan added a comment.Dec 11 2017, 10:08 PM
Executing section Default...
Executing section Source...
Executing section JS...
Harbormaster completed remote builds in B4094: Diff 4722.Dec 11 2017, 10:08 PM
wraitii added subscribers: Itms, wraitii.Dec 12 2017, 9:47 AM

@Itms That "successful build" seems rather unsuccessful to me.

Vulcan requested changes to this revision.Feb 10 2018, 10:10 PM
This comment was removed by phabadmin.
This revision now requires changes to proceed.Feb 10 2018, 10:10 PM
phabadmin added a subscriber: phabadmin.Feb 10 2018, 10:11 PM
Itms removed a reviewer: Vulcan.Feb 10 2018, 10:13 PM
Itms removed subscribers: phabadmin, Vulcan.

Sorry, wrong manipulation...

Itms requested changes to this revision.Feb 20 2018, 7:26 PM
Itms edited reviewers, added: Itms; removed: • leper, Restricted Owners Package.

I'm on it as part of my D1029 review. Thanks for your work, it will be awesome to have this! ?

source/main.cpp
498 ↗(On Diff #4722)

The biggest concern here is whether this can be integrated with desktop environments (by "opening" the pyromod file directly). The usual way of doing that is by calling pyrogenesis.exe mod.pyromod so it would be better to open mods like this, rather than using a funky command-line option. Does CmdLineArgs already support that? It would need to support that at some point anyways.

513 ↗(On Diff #4722)

Maybe check here that the basename is non-empty and the extension is correct (either zip or pyromod), else print that the filename is "invalid". Right now I find the error message a bit confusing.

source/ps/ModInstaller.cpp
38 ↗(On Diff #4722)

I think this shouldn't part of the mod installer, but should be part of the mod installation startup chain. Indeed, if we want to call the mod installer from the mod loader, a script interface will already be present. I think a const ScriptInterface& scriptInterface should be passed to CModInstaller::Install.

57 ↗(On Diff #4722)

See my comment below.

90 ↗(On Diff #4722)

We don't seem to have anything for copy so I'm OK with using Boost for that, but I'd suggest creating a CopyFile that uses Boost in source/lib/file/file_system.{h,cpp}. That way it is available for other purposes.

93 ↗(On Diff #4722)

Maybe we could extract the file on all platforms for consistency but I don't have a strong opinion.

103 ↗(On Diff #4722)

wunlink

Imarok added a subscriber: Imarok.Mar 24 2018, 9:41 AM
Imarok added inline comments.
source/ps/ModInstaller.cpp
90 ↗(On Diff #4722)

Isn't wrename the same as copy?

vladislavbelov added inline comments.Mar 24 2018, 11:53 AM
source/ps/ModInstaller.cpp
90 ↗(On Diff #4722)

No, copy doesn't delete the old named file. The rename is equal to move.

Imarok added inline comments.Mar 24 2018, 11:59 AM
source/ps/ModInstaller.cpp
90 ↗(On Diff #4722)

true ^^

vladislavbelov added a comment.Apr 8 2018, 4:14 PM

The diff is pretty old, all new changes are in a separate branch, the diff will be updated/closed later.

vladislavbelov planned changes to this revision.Apr 8 2018, 4:14 PM
Itms commandeered this revision.Apr 15 2018, 1:41 AM
Itms edited reviewers, added: vladislavbelov; removed: Itms.
Itms updated this revision to Diff 6394.Apr 15 2018, 1:45 AM
Itms edited the test plan for this revision. (Show Details)

I imported this version from our private branch. I only made a few English changes in comments and one bugfix.

Itms added a comment.Apr 15 2018, 1:51 AM

Here are my own changes

binaries/system/readme.txt
55 ↗(On Diff #6394)

Reworded.

source/main.cpp
505 ↗(On Diff #6394)

Added output.

631 ↗(On Diff #6394)

Bugfix: this was missing and made the mod selector unescapable ?

source/ps/GameSetup/CmdLineArgs.cpp
27 ↗(On Diff #6394)

Renamed.

source/ps/ModInstaller.h
27 ↗(On Diff #6394)

Reworded most Doxygen comments

69 ↗(On Diff #6394)

Moved the definition here so that there is no need to specify it is static in a comment.

Vulcan added a comment.Apr 15 2018, 2:01 AM

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|  33| #include·"ps/Game.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|  33| #include·"ps/Game.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|  33| #include·"ps/Game.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'OS_MACOSX'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|  33| #include·"ps/Game.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'OS_WIN'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|  33| #include·"ps/Game.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
| 350| 350| 
| 351| 351| 	g_ModsEnabled.sort((folder1, folder2) =>
| 352| 352| 		dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 :
| 353|    |-		dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
|    | 353|+			dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
| 354| 354| 
| 355| 355| 	displayModList("modsEnabledList", g_ModsEnabled);
| 356| 356| }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'Engine'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
| 365| 365| 	{
| 366| 366| 		otherListObject.selected = -1;
| 367| 367| 		Engine.GetGUIObjectByName("visitWebButton").enabled = true;
| 368|    |-		let disEnableButton =  Engine.GetGUIObjectByName("disEnableButton")
|    | 368|+		let disEnableButton = Engine.GetGUIObjectByName("disEnableButton")
| 369| 369| 		disEnableButton.caption = listObjectName == "modsDisabledList" ? "Enable" : "Disable";
| 370| 370| 		disEnableButton.enabled = true;
| 371| 371| 		disEnableButton.onPress = listObjectName == "modsDisabledList" ? enableMod : disableMod;
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
| 365| 365| 	{
| 366| 366| 		otherListObject.selected = -1;
| 367| 367| 		Engine.GetGUIObjectByName("visitWebButton").enabled = true;
| 368|    |-		let disEnableButton =  Engine.GetGUIObjectByName("disEnableButton")
|    | 368|+		let disEnableButton =  Engine.GetGUIObjectByName("disEnableButton");
| 369| 369| 		disEnableButton.caption = listObjectName == "modsDisabledList" ? "Enable" : "Disable";
| 370| 370| 		disEnableButton.enabled = true;
| 371| 371| 		disEnableButton.onPress = listObjectName == "modsDisabledList" ? enableMod : disableMod;

binaries/data/mods/mod/gui/modmod/modmod.js
| 368| »   »   let·disEnableButton·=··Engine.GetGUIObjectByName("disEnableButton")
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

Link to build: https://jenkins.wildfiregames.com/job/differential/385/display/redirect

Harbormaster failed remote builds in B5866: Diff 6394!Apr 15 2018, 2:01 AM
Vulcan added a comment.Apr 15 2018, 3:42 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|  33| #include·"ps/Game.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|  33| #include·"ps/Game.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|  33| #include·"ps/Game.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'OS_MACOSX'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|  33| #include·"ps/Game.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'OS_WIN'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|  33| #include·"ps/Game.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
| 350| 350| 
| 351| 351| 	g_ModsEnabled.sort((folder1, folder2) =>
| 352| 352| 		dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 :
| 353|    |-		dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
|    | 353|+			dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
| 354| 354| 
| 355| 355| 	displayModList("modsEnabledList", g_ModsEnabled);
| 356| 356| }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'Engine'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
| 365| 365| 	{
| 366| 366| 		otherListObject.selected = -1;
| 367| 367| 		Engine.GetGUIObjectByName("visitWebButton").enabled = true;
| 368|    |-		let disEnableButton =  Engine.GetGUIObjectByName("disEnableButton")
|    | 368|+		let disEnableButton = Engine.GetGUIObjectByName("disEnableButton")
| 369| 369| 		disEnableButton.caption = listObjectName == "modsDisabledList" ? "Enable" : "Disable";
| 370| 370| 		disEnableButton.enabled = true;
| 371| 371| 		disEnableButton.onPress = listObjectName == "modsDisabledList" ? enableMod : disableMod;
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js
| 365| 365| 	{
| 366| 366| 		otherListObject.selected = -1;
| 367| 367| 		Engine.GetGUIObjectByName("visitWebButton").enabled = true;
| 368|    |-		let disEnableButton =  Engine.GetGUIObjectByName("disEnableButton")
|    | 368|+		let disEnableButton =  Engine.GetGUIObjectByName("disEnableButton");
| 369| 369| 		disEnableButton.caption = listObjectName == "modsDisabledList" ? "Enable" : "Disable";
| 370| 370| 		disEnableButton.enabled = true;
| 371| 371| 		disEnableButton.onPress = listObjectName == "modsDisabledList" ? enableMod : disableMod;

binaries/data/mods/mod/gui/modmod/modmod.js
| 368| »   »   let·disEnableButton·=··Engine.GetGUIObjectByName("disEnableButton")
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

Link to build: https://jenkins.wildfiregames.com/job/differential/386/display/redirect

Harbormaster completed remote builds in B5866: Diff 6394.Apr 15 2018, 3:42 AM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 15 2018, 3:47 AM
Closed by commit rP21726: Add a mod installer, fixes #4027. (authored by Itms). · Explain Why
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Apr 15 2018, 3:47 AM
ffffffff added a subscriber: ffffffff.May 27 2018, 9:47 PM

failes install pyromod when ~/.cache/0ad is pointing symbolic linked to another filesystem (i was using an aufs mounted fs on /mnt but into a folder where user can write. It can actually make a fgod folder temporary inside but then moving files to correct location under ~/.local/mods/0ad/mods/fgod failes so far (ps/trunk/source/ps/ModInstaller.cpp line 83).

ps/trunk/source/ps/ModInstaller.cpp
83

fail here

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
mod/
gui/
modmod/
22 lines
system/
3 lines
source/
lib/
file/
4 lines
24 lines
41 lines
ps/
GameSetup/
14 lines
56 lines
3 lines
6 lines
tests/
21 lines
82 lines
111 lines
tools/
atlas/
GameInterface/
Handlers/
2 lines

Diff 6395

View Options

ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.js

Loading...
View Options

ps/trunk/binaries/system/readme.txt

Loading...
View Options

ps/trunk/source/lib/file/file_system.h

Loading...
View Options

ps/trunk/source/lib/file/file_system.cpp

Loading...
View Options

ps/trunk/source/main.cpp

Loading...
View Options

ps/trunk/source/ps/GameSetup/CmdLineArgs.h

Loading...
View Options

ps/trunk/source/ps/GameSetup/CmdLineArgs.cpp

Loading...
View Options

ps/trunk/source/ps/GameSetup/GameSetup.h

Loading...
View Options

ps/trunk/source/ps/GameSetup/GameSetup.cpp

Loading...
View Options

ps/trunk/source/ps/GameSetup/tests/test_CmdLineArgs.h

Loading...
View Options

ps/trunk/source/ps/ModInstaller.h

Loading...
View Options

ps/trunk/source/ps/ModInstaller.cpp

Loading...
View Options

ps/trunk/source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp

Loading...
Wildfire Games · Phabricator