Provides updated PackageManager architecture #452

praj-foss · 2019-08-17T23:33:41Z

Summary

Provides an improved architecture for the PackageManager, adding features to support multiple online repositories and better handling of game packages. Fixes #451.

This architecture was inspired by classic Linux package managers like apt and pacman. The design was originally planned as shown in this diagram:

Testing

Coming soon

Outstanding before merging

Add PackageDatabase support
Handle repositories mentioned in a local file
Write/read the database to/from a local file
Handle multiple repository types
Integrate with old codebase and PackageManager

- PackageDatabase works with repositories mentioned in a file - Adds methods to sync, load and save PackageDatabase - Adds RepositoryHandlers for handling Jenkins and custom repositories

GooeyHub · 2019-08-17T23:36:29Z

Hooray Jenkins reported success with all tests good!

- Package names need to be mentioned in sources.json file for tracking - Packages contains location field for locating their ZIPs

GooeyHub · 2019-08-21T12:42:50Z

Hooray Jenkins reported success with all tests good!

- Presents database as a list of package instances - Package instances provide isInstalled() method - Changes some PackageManager and RespositoryHandler methods for consistency

- Replaces TerasologyGameVersions instances with PackageManager in controllers - Minor changes to PackageManager API

GooeyHub · 2019-10-11T08:10:42Z

Hooray Jenkins reported success with all tests good!

- Uses DownloadUtils and FileUtils inside PackageManager to download and extract packages - Provides DownloadTask inside ApplicationController to observe installation progress - Moves directory name strings from PackageDatabase to PackageManager - PackageDatabase detects installed packages during sync()

GooeyHub · 2019-10-21T20:40:54Z

Hooray Jenkins reported success with all tests good!

- Modifies GameStarter to start games installed via PackageManager

GooeyHub · 2019-10-22T04:26:49Z

Hooray Jenkins reported success with all tests good!

- PackageManager directly installs the cached package if available - Disables combo-boxes while downloading

- PackageManager removes specified game package - Defines DeleteTask to perform delete operation asynchronously

GooeyHub · 2019-10-22T07:36:51Z

Hooray Jenkins reported success with all tests good!

- Copies the supplied sources.json file when it's absent in the local filesystem - Changes config directory structure - Removes all toAbsolutePath() calls during logging in PackageDatabase

- Prevents download button from getting buggy after closing settings menu - Binds package and version combo-boxes of both the controllers

GooeyHub · 2019-10-25T14:18:04Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2019-11-02T11:17:28Z

Hooray Jenkins reported success with all tests good!

The utility methods for directories are very specific for the launcher and even contain constants for the data directory name, cache directory, or application directory name. The new name should indicate this dedicated purpose. `FileUtils` are more general and not tightly coupled to launcher functionality. Some methods might be moved there in the future.

This is a replacement for `FileUtils.deleteDirectoryContent` which additionally ensures that the directory exists and is empty. The old method has been made private and is still used internally. - updated call to use `ensureEmptyDir` - added tests for `ensureEmptyDir`

…ls.ensureWritableDir` - introduce `LauncherManagedDirectory` enum for directories local to launcher installation path - introduce `DirectoryCreator` functional interface - refactor `getXYZDirectory` methods in the init task - make use of managed directory enum - make use of DirectoryCreator interface via method references - unify directory creation and error handling

GooeyHub · 2019-11-02T15:15:59Z

Uh oh, something went wrong with the build. Need to check on that

GooeyHub · 2019-11-02T15:39:42Z

Hooray Jenkins reported success with all tests good!

- use `!Files.exists(dir)` instead of `Files.notExists(dir)` like in ensureWritableDir - add `Files.isDirectory(dir)` check

GooeyHub · 2019-11-03T07:52:56Z

Hooray Jenkins reported success with all tests good!

jdrueckert · 2019-11-03T07:55:50Z

src/main/java/org/terasology/launcher/util/FileUtils.java

+            if (!Files.isDirectory(directory)) {
+                throw new IOException("Directory is not a directory! " + directory);
+            } else {


I'm not entirely sure, if we want to put that here.
deleteDirectoryContent will throw a NotDirectoryException on calling Files.list(directory) if directory is not a directory. While this should be a well-understandable exception, I think it makes sense to check before-hand.
I put the check into ensureEmptyDir to align it with ensureWritableDir which includes this check as well, but maybe it would be better off in deleteDirectoryContent...
@praj-foss @skaldarnar what do you think?

This needs a revisit anyways. Some notes on the current state and where it might be headed:

Files.createDirectories(path) will create directory (if possible) or return the directory if it exists. If it is no directory, it will throw an exception - pretty much what we are trying to do as a first step here and in ensureWritableDir. Maybe we can make better use of it.

I'm not really happy with the void return types and the exceptional behavior. Maybe we can hide that and instead expose Optional<Path> or the like for internal usage (as abstraction over the underlying file system operations).

Seems like we are building this up in multiple steps: (1) ensure directory exists, (2) ensure directory is writable, (3) ensure directory is empty, ... Not sure how we could build a nice functional interface for that, or whether it is actually worth it 🙄

... and file attributes: https://docs.oracle.com/javase/8/docs/api/java/nio/file/attribute/package-summary.html 🤷‍♂

GooeyHub · 2019-11-03T09:01:14Z

Uh oh, something went wrong with the build. Need to check on that

GooeyHub · 2019-11-03T09:27:02Z

Hooray Jenkins reported success with all tests good!

skaldarnar

@praj-foss @jdrueckert Left a bunch of comments such that we know where to continue 😉 Will hit that green button now and finally merge it, should make it easier to progress in smaller steps towards a new release.

skaldarnar · 2019-11-03T20:15:53Z

src/main/java/org/terasology/launcher/LauncherInitTask.java

-    private Path getLauncherDirectory(OperatingSystem os) throws LauncherStartFailedException {
-        logger.trace("Init LauncherDirectory...");
-        final Path launcherDirectory = DirectoryUtils.getApplicationDirectory(os, DirectoryUtils.LAUNCHER_APPLICATION_DIR_NAME);
+    private void initDirectory(Path dir, String errorLabel, DirectoryCreator... creators)


The whole set of methods dealing with directory creation, initialization, etc need to get a revisit. Cleaning up naming, adding Javadoc, refactoring (more) common code.

skaldarnar · 2019-11-03T20:17:13Z

src/main/java/org/terasology/launcher/LauncherInitTask.java

@@ -285,20 +275,6 @@ private Path getGameDataDirectory(OperatingSystem os, Path settingsGameDataDirec
        return gameDataDirectory;
    }

-    private TerasologyGameVersions getTerasologyGameVersions(Path launcherDirectory, Path gameDirectory, BaseLauncherSettings launcherSettings) {


Not sure whether this should be removed already (@praj-foss ?). I just threw it out because it was not used... Can re-add it easily from Git history if needed, though.

skaldarnar · 2019-11-03T20:18:59Z

src/main/java/org/terasology/launcher/game/TerasologyGameVersions.java

@@ -93,7 +93,7 @@ public synchronized TerasologyGameVersion getGameVersionForBuildVersion(GameJob
    }

    public synchronized void loadGameVersionsFromPackageManager(GameSettings gameSettings, Path launcherDirectory, Path gameDirectory) {
-        final Path cacheDirectory = launcherDirectory.resolve(DirectoryUtils.CACHE_DIR_NAME);
+        final Path cacheDirectory = launcherDirectory.resolve(LauncherDirectoryUtils.CACHE_DIR_NAME);


I think all the launcher managed directories should be retrieved from the config/settings object, instead of resolving the path again and again.

I agree with that.. It should look more sensible to have the PackageManager directly depend on the settings object and resolve all required paths itself.

skaldarnar · 2019-11-03T20:21:20Z

src/main/java/org/terasology/launcher/gui/javafx/ApplicationController.java

@@ -89,20 +90,100 @@
    private static final long MB = 1024L * 1024;
    private static final long MINIMUM_FREE_SPACE = 200 * MB;


I guess this should more wide be used, or thrown out 🤔 maybe even a combination of (a) minimum absolute free space (e.g., 200mb) and (b) not more than 95% of disk space used...

skaldarnar · 2019-11-03T20:21:44Z

src/main/java/org/terasology/launcher/gui/javafx/ApplicationController.java

@@ -89,20 +90,100 @@
    private static final long MB = 1024L * 1024;
    private static final long MINIMUM_FREE_SPACE = 200 * MB;

+    private static final class DownloadTask extends Task<Void> implements ProgressListener {


Definitely needs Javadoc, and should probably live in its own class.

+1 for Javadoc.. But won't this task be better encapsulated as an inner class, if it's not supposed to be used outside of ApplicationController?

skaldarnar · 2019-11-03T20:36:40Z

src/main/java/org/terasology/launcher/packages/RepositoryHandler.java

+/**
+ * Provides method to fetch all packages from a repository.
+ */
+interface RepositoryHandler {


Should this be a @FunctionalInterface?

Sometimes I really forget that I'm on Java 8 😅

skaldarnar · 2019-11-03T20:37:22Z

src/main/java/org/terasology/launcher/util/DirectoryCreator.java

+/**
+ * Functional interface to abstract over directory creation.
+ */
+public interface DirectoryCreator {


This should have @FunctionalInterface annotation

skaldarnar · 2019-11-03T20:52:31Z

src/main/java/org/terasology/launcher/util/FileUtils.java

+            if (!Files.isDirectory(directory)) {
+                throw new IOException("Directory is not a directory! " + directory);
+            } else {


This needs a revisit anyways. Some notes on the current state and where it might be headed:

Files.createDirectories(path) will create directory (if possible) or return the directory if it exists. If it is no directory, it will throw an exception - pretty much what we are trying to do as a first step here and in ensureWritableDir. Maybe we can make better use of it.

I'm not really happy with the void return types and the exceptional behavior. Maybe we can hide that and instead expose Optional<Path> or the like for internal usage (as abstraction over the underlying file system operations).

Seems like we are building this up in multiple steps: (1) ensure directory exists, (2) ensure directory is writable, (3) ensure directory is empty, ... Not sure how we could build a nice functional interface for that, or whether it is actually worth it 🙄

... and file attributes: https://docs.oracle.com/javase/8/docs/api/java/nio/file/attribute/package-summary.html 🤷‍♂

skaldarnar · 2019-11-03T20:53:11Z

src/main/java/org/terasology/launcher/util/LauncherDirectoryUtils.java


    public static final String LAUNCHER_APPLICATION_DIR_NAME = "TerasologyLauncher";
    public static final String GAME_APPLICATION_DIR_NAME = "Terasology";
    public static final String GAME_DATA_DIR_NAME = "Terasology";
-    public static final String DOWNLOAD_DIR_NAME = "download";
-    public static final String TEMP_DIR_NAME = "temp";
    public static final String CACHE_DIR_NAME = "cache";


I think this can be removed .... thought I did it, now have to do it later 😅

skaldarnar · 2019-11-03T20:54:23Z

src/main/resources/org/terasology/launcher/bundle/LabelsBundle.properties

@@ -75,6 +75,7 @@ message_error_loadSettings=
 message_error_operatingSystem=
 message_error_storeSettings=
 message_error_tempDirectory=
+message_error_cacheDirectory=


need to add actual text somewhere 🙄 double check if there is an error label w.r.t. to download directory.

replaced by MovingBlocks#452

Provides PackageDatabase and RepositoryHandlers

21fc2eb

- PackageDatabase works with repositories mentioned in a file - Adds methods to sync, load and save PackageDatabase - Adds RepositoryHandlers for handling Jenkins and custom repositories

praj-foss added the GSoC 2019 label Aug 17, 2019

praj-foss self-assigned this Aug 17, 2019

praj-foss added this to the v4.0.0 - GSOC 2019 milestone Aug 17, 2019

Provides implementation of JenkinsHandler

863ff85

- Package names need to be mentioned in sources.json file for tracking - Packages contains location field for locating their ZIPs

praj-foss added 2 commits October 10, 2019 23:33

Modifies Package database structure

e6f8953

- Presents database as a list of package instances - Package instances provide isInstalled() method - Changes some PackageManager and RespositoryHandler methods for consistency

Disconnects UI from TerasologyGameVersions

771def0

- Replaces TerasologyGameVersions instances with PackageManager in controllers - Minor changes to PackageManager API

Support for running installed games

6c4fbc3

- Modifies GameStarter to start games installed via PackageManager

praj-foss added 2 commits October 22, 2019 11:45

Support for handling cached packages

dbd011f

- PackageManager directly installs the cached package if available - Disables combo-boxes while downloading

Support for removing installed packages

8216246

- PackageManager removes specified game package - Defines DeleteTask to perform delete operation asynchronously

praj-foss requested review from skaldarnar, Cervator and jdrueckert October 22, 2019 11:57

praj-foss added 2 commits October 25, 2019 17:55

Adds default sources file

43de0aa

- Copies the supplied sources.json file when it's absent in the local filesystem - Changes config directory structure - Removes all toAbsolutePath() calls during logging in PackageDatabase

Fixes main UI issues on closing settings menu

9881985

- Prevents download button from getting buggy after closing settings menu - Binds package and version combo-boxes of both the controllers

Merge branch 'develop' into update/package-manager

2ab26cf

skaldarnar added 4 commits November 2, 2019 12:43

Javadoc and formatting for LauncherConfiguration

34ec4b9

remove unused method in LauncherInitTask

a2651cd

move tests to TestFileUtils and setup mocking context

a175e35

Align ensureEmptyDir with ensureWritableDir

31565c1

- use `!Files.exists(dir)` instead of `Files.notExists(dir)` like in ensureWritableDir - add `Files.isDirectory(dir)` check

jdrueckert reviewed Nov 3, 2019

View reviewed changes

jdrueckert added 2 commits November 3, 2019 09:00

Add cacheDirectory to LauncherConfiguration

b665f0e

Fix NullLauncherConfiguration

e843e71

skaldarnar approved these changes Nov 3, 2019

View reviewed changes

skaldarnar merged commit ffd1120 into develop Nov 3, 2019

skaldarnar deleted the update/package-manager branch November 3, 2019 21:04

praj-foss mentioned this pull request Nov 9, 2019

Proper handling of configuration file #461

Closed

2 tasks

This was referenced Nov 10, 2019

Wrong version identifier is shown in the UI #467

Closed

"About" tab is empty #465

Closed

Changelog of selected game not shown #464

Closed

keturn added a commit to keturn/TerasologyLauncher that referenced this pull request Apr 30, 2020

chore: remove unused GameDownloader

74855d1

replaced by MovingBlocks#452

keturn mentioned this pull request Apr 30, 2020

chore: remove unused GameDownloader #555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provides updated PackageManager architecture #452

Provides updated PackageManager architecture #452

praj-foss commented Aug 17, 2019 •

edited

Loading

GooeyHub commented Aug 17, 2019

GooeyHub commented Aug 21, 2019

GooeyHub commented Oct 11, 2019

GooeyHub commented Oct 21, 2019

GooeyHub commented Oct 22, 2019

GooeyHub commented Oct 22, 2019

GooeyHub commented Oct 25, 2019

GooeyHub commented Nov 2, 2019

GooeyHub commented Nov 2, 2019

GooeyHub commented Nov 2, 2019

GooeyHub commented Nov 3, 2019

jdrueckert Nov 3, 2019

skaldarnar Nov 3, 2019

GooeyHub commented Nov 3, 2019

GooeyHub commented Nov 3, 2019

skaldarnar left a comment

skaldarnar Nov 3, 2019

skaldarnar Nov 3, 2019

skaldarnar Nov 3, 2019

praj-foss Nov 9, 2019

skaldarnar Nov 3, 2019

skaldarnar Nov 3, 2019

praj-foss Nov 9, 2019

skaldarnar Nov 3, 2019

praj-foss Nov 9, 2019

skaldarnar Nov 3, 2019

skaldarnar Nov 3, 2019

skaldarnar Nov 3, 2019

skaldarnar Nov 3, 2019

		@@ -89,20 +90,100 @@
		private static final long MB = 1024L * 1024;
		private static final long MINIMUM_FREE_SPACE = 200 * MB;

Provides updated PackageManager architecture #452

Provides updated PackageManager architecture #452

Conversation

praj-foss commented Aug 17, 2019 • edited Loading

Summary

Testing

Outstanding before merging

GooeyHub commented Aug 17, 2019

GooeyHub commented Aug 21, 2019

GooeyHub commented Oct 11, 2019

GooeyHub commented Oct 21, 2019

GooeyHub commented Oct 22, 2019

GooeyHub commented Oct 22, 2019

GooeyHub commented Oct 25, 2019

GooeyHub commented Nov 2, 2019

GooeyHub commented Nov 2, 2019

GooeyHub commented Nov 2, 2019

GooeyHub commented Nov 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GooeyHub commented Nov 3, 2019

GooeyHub commented Nov 3, 2019

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praj-foss commented Aug 17, 2019 •

edited

Loading