PhoenicisOrg / phoenicis

Phoenicis PlayOnLinux and PlayOnMac 5 repository
https://phoenicis.org/
GNU Lesser General Public License v3.0
666 stars 73 forks source link

Create a new interface so that we can instanciate different kind of repositories #1723

Open qparis opened 5 years ago

plata commented 5 years ago

My understanding: A repository contains a script which can dynamically populate the repository or a new repository. Example: The Wine script populates the repository with all available Wine versions. With this approach, the apps tab implementation could be reused for the engines tab.

Other possible use cases:

qparis commented 5 years ago

So you are introducing dependencies between repositories

plata commented 5 years ago

Or extend a repository. What did you have in mind?

plata commented 5 years ago

Or we have a new RepositoryGenerator type which is a single Javascript that can be like a repository.

qparis commented 5 years ago

Or inside a repository, we can add a optional set of class that will "decorate" the way the repository is loaded. Then, we create a separate git repository for all the new type of scripts

plata commented 5 years ago

The possible issue I see with this is the following: Where to put the generated scripts in the repository structure? You cannot allow to put it in the existing structure because it might overwrite existing scripts. If you use a separate repository which is completely generated, you solve this issue.

qparis commented 5 years ago

We add " - Plugins" directory. So it would be like

Inside a plugin, you'll find two interface implemntations (for now) like:

Then repository can extends has many interface like this they want

plata commented 5 years ago

I don't quite see how this would behave yet. Can you give an example?

qparis commented 5 years ago

When the backend loads a repositorys, it does two things AFAIK:

plata commented 5 years ago

How does this add categories/applications to the repository?

qparis commented 5 years ago

By hooking TypeDTO (that contains Categories and games)

plata commented 5 years ago

Ok. If you have multiple repositories, the hook will apply to all previously loaded?

qparis commented 5 years ago

No, just the current one

plata commented 5 years ago

That should work. And it allows us to e.g. use naive Steam scripts only for cases where we do not have another script already.

I don't see the use cases for the decorators yet.

qparis commented 5 years ago

Let's imagine that you have a JSON script in your repository (before or after the hook is applied, it is not important).

Then, the backend will execute decorator1(decorator2(decorator3(decorator4(json_script))), instead of json_script

plata commented 5 years ago

I understand how it works but not what it could be used for.

qparis commented 5 years ago

That should work. And it allows us to e.g. use naive Steam scripts only for cases where we do not have another script already.

Keep in mind that the hooks only have the current repository as input to avoid side effect

I understand how it works but not what it could be used for.

Running JSON script without having an interpreter

plata commented 5 years ago

Running JSON script without having an interpreter

So you would store Lutris scripts in the repository directly and then use the decorator at runtime? My idea was to create a Phoenicis format in the population hook already.

qparis commented 5 years ago

Running JSON script without having an interpreter

So you would store Lutris scripts in the repository directly and then use the decorator at runtime? My idea was to create a Phoenicis format in the population hook already.

One class, one responsibility. In addition, you'll have a lot of duplicated code if you do like this. And also, this will allow to support home made YAML/JSON/any kind of format script

plata commented 5 years ago

With your approach, the Lutris yaml must be downloaded inside a script.js, correct?

qparis commented 5 years ago

I don't think so because ScriptDTO contains a URI to the script content. So I'm not sure

plata commented 5 years ago

In this case it will never work offline.

qparis commented 5 years ago

Phoenicis automatically caches the URLs, right?

plata commented 5 years ago

It caches the git repository. If the hook does not create files but only DTO objects in the repository tree, they will not be there.

qparis commented 5 years ago

So how does it work between the String source and the URL source?

plata commented 5 years ago

What do mean by "String source" and "URL source"?

qparis commented 5 years ago

If you look at ScriptDTO, you have two fields: private final String script; private final URI scriptSource;

plata commented 5 years ago

Yes but that's only the DTO. If you close Phoenicis, it's gone.

qparis commented 5 years ago

Ok. Maybe we can improve the caching part later then

plata commented 5 years ago

It doesn't really change it. Then you will just write it to that file in the cache. Additionally, I think it breaks the concept because it avoids that you can always run the scripts in a repository. Let's say you have two repositories with Lutris scripts. Then you will always need the decorators in both repositories.

qparis commented 5 years ago

Yes. But introducing dependencies between repositories is very hazardous. Imagine someone creates a hook that breaks an official script and we get report about this

qparis commented 5 years ago

The caching issue should be treated globally. If we have a HTTP repository with HTTP urls for scripts, the caching needs to be supported. If I understand, we are currently relying on local git to support the cache, which is I think a problem

plata commented 5 years ago

Regardless how we realize the caching: I don't see the advantage of caching the yaml instead of a script based on the yaml which would have the advantage that you can run it directly.

qparis commented 5 years ago

I feel uncomfortable with he idea of transpiling source code. It smells hacky

plata commented 5 years ago

We don't have to do that. I was thinking more of something like you did in the POC PR:

include(["engines", "wine", "quick_script", "lutris_wine_steam_script"]);

var installerImplementation = {
    run: function () {
        new LutrisWineSteamScript()
            .yaml("yaml content here")
            .go();
    }
};

/* exported Installer */
var Installer = Java.extend(org.phoenicis.scripts.Installer, installerImplementation);
qparis commented 5 years ago

So you are kind of transpiling, because you are generating some JS Files. With my solution, no JS file is generated at all

plata commented 5 years ago

I know but in my opinion that's rather a problem because you save a yaml in a js and you cannot execute some of the scripts (more or less randomly).

qparis commented 5 years ago

You can because you have the hook installed.

I mean, if you generate JS files, your method is very tied to the language of the scripts. Your generated code will be stored inside scripts, so codacy will not apply, the refactor function will not work (Imagine you rename some of the imports you are using in the generated script, ...)

Etc..

Generation of code should be generally avoided I think

qparis commented 5 years ago

Not to mention security problem. What if we have a .yaml file that contains crafted JS injection

plata commented 5 years ago

How would it work for the Wine engine versions?

qparis commented 5 years ago

What do you mean?

plata commented 5 years ago

Remember that the idea was also to use this mechanism for Wine versions such that we could reuse the apps tab implementation?

qparis commented 5 years ago

I don't remember that

qparis commented 5 years ago

@plata @madoar What do we decide here?

plata commented 5 years ago

I don't know yet. I'm not really convinced by any of the solutions.

qparis commented 5 years ago

I dont see what could be the problem if we decide to store the scripts in another format.

plata commented 5 years ago

It's not the format (we could rename it from .js and I would be fine with that).

I think my issue is somewhere else: With this approach, the meaning of script.js is not well defined anymore. Currently, it is a script which is triggered from the embedding application but then is in charge of the execution control. If the script is only e.g. a yaml, it's meaning changes logically because it's suddenly no longer executing but is executed (by the decorator). Mixing these two approaches doesn't feel good for me.

Maybe we should just avoid to cache these external, dynamically created parts of the repository. Then we could simply use the populate hook to create executable scripts from yaml like I've described in https://github.com/PhoenicisOrg/phoenicis/issues/1723#issuecomment-451653839. Redundancy would not be a problem because all the logic is inside the hook. Additionally, it would allow us to create installable engine version scripts which could be used in the same way as apps.

qparis commented 5 years ago

It's not the format (we could rename it from .js and I would be fine with that).

I think my issue is somewhere else: With this approach, the meaning of script.js is not well defined anymore. Currently, it is a script which is triggered from the embedding application but then is in charge of the execution control. If the script is only e.g. a yaml, it's meaning changes logically because it's suddenly no longer executing but is executed (by the decorator). Mixing these two approaches doesn't feel good for me.

The best approach is to consider that a script is executed. This is a good IoC principle we should apply. By the way, this is what you have started to do by enforcing script to extend objects

Maybe we should just avoid to cache these external, dynamically created parts of the repository. Then we could simply use the populate hook to create executable scripts from yaml like I've described in [#1723

This approach is very dangerous.

plata commented 5 years ago

I would like to hear @madoar opinion as well.

madoar commented 5 years ago

@qparis I don't understand what you would like to populate a repository with?

@plata My idea of Phoenicis is, that it should stay in control at all times. Ideally this means, that the every command executed inside a "script" should be checked/performed by Phoenicis.

The reason why I think this is, that this approach leads to more security and control. Our scripts are so generic, that you can do anything with them. This in turn makes it very difficult to find bugs later on. In addition it makes it quite easy for script developers to inject some malicious commands into their scripts.

In addition I think Phoenicis should only support one type/format of script officially. Everything else should be done by the community externally from Phoenicis. Otherwise it can become a nightmare to maintain this. This means that if there are people who want to have direct/native support of lutris scripts in Phoenicis, they should write a solution outside of Phoenicis. This is also the reason for my idea of a converter for lutris scripts, because such a converter could exist outside of Phoenicis.