API Discussion about libsu HOT 20 CLOSED

@PerfectSlayer so I've made some changes after your suggestions.

• enqueue is renamed to submit. The original idea is from JobIntentService, but I personally also prefer the Java Executor way
• Removed Job.onResult and add an overload Job.submit(ResultCallback)

I added chaining operations, so the final Job class is like this:

public abstract static class Job {
    public abstract Job to(List<String> stdout);
    public abstract Job to(List<String> stdout, List<String> stderr);
    public abstract Job add(InputStream in);
    public abstract Job add(String... cmds);
    public abstract Result exec();
    public abstract void submit();
    public abstract void submit(ResultCallback cb);
}

Along with these changes two non-static APIs in Shell are replaced with a new one:

// Add
public abstract Job newJob();
// Remove
public abstract Job newJob(InputStream in);
public abstract Job newJob(String... cmds);

To directly interact with a Shell instance will be like
shell.newJob().add(InputStream).add(cmds....).add(...).exec()

However, I did not touch the high level APIs, because in most cases people will just use Shell.su(cmds...) or Shell.su(in) (at least in my case). New operations can also be added to the returned Job from the high level APIs, so nothing is missing there.

Also, I personally do not like method names to be too verbose (contradict with common Java code naming conventions LOL), so I named my API very simple, like add and to. Also I would love to leave the to(stdout) and to(stdout, stderr) methods untouched too, because in the case of to(stdout), it will check the global flag FLAG_REDIRECT_STDERR to decide whether STDERR should be collected. To only collect STDERR, just use to(null, List).

from libsu.

@victorlapin there are still bugs in the old versions and I don't want to maintain two branches alongside.
All methods and classes in the shim are annotated @Deprecated, so developers using a decent IDE will be notified to use the newer API.

The shim is fairly simple so it wouldn't be too complicated to maintain, since the previous API is mostly a collection of method overrides and actually only rely on a few key methods. I just need to transform the few key methods and it's good to go!

libsu is meant to have 0 dependencies itself so I kept it nice and clean. Creating RxJava2 wrapper is definitely very cool! I think it is also possible to wrap it with Support Library's Lifecycle-Aware Components. I'll leave these to other developers though, my main goal should be making this library as robust as possible!

from libsu.

Hello @PerfectSlayer, thanks for all your suggestions! I'm actually quite new to Java development, and I appreciate receiving advice from others 👍

My library is inspired by Chainfire's libsuperuser. The biggest difference I'm aiming for is a single root shell that is shared by all operations across the app. Most of the design decision is based on my own personal needs in Magisk Manager and cues from libsuperuser.

I agree with your opinion that an Output class will be a good idea (and the return code). Currently my idea is to have the class be like:

class Output {
  List<String> outList;
  List<String> errList;
  int returnCode;
}

However, it is not possible to merge all sh/su(.....) to a single Output sh/su(cmds...), because in many cases, the outList and errList has to be passed in for special purposes, for example updating the UI along with shell outputs by using CallbackList (check my example module).
So my idea would be these 2 methods:

• Output sh/su(String...) to let the method construct an Output for you
• void sh/su(String..., Output) to let the shell directly output to the passed in Output instance

(This also fixes the parameter positioning you also mentioned)

For Shell.Initializer, yes we can actually check root access with shell.getStatus() >= ROOT_SHELL, so onShellInit and onRootShellInit is indeed duplicated in a way, I will consider removing one. About throwing an Exception and returning boolean: any Exception means the initialization has an error; it is also possible that something other than exceptions happens, and the developer would like to mark the initialization as failure so I also allowed returning false. You can argue that developers can catch the Exceptions themselves, and return false in the catch block if they want, but I just think it is handy to have both options available. Not sure if that actually created more confusion, I would love your thoughts here.

Shell.Task, execTask(Task) is meant to be used for low level implementation, it opens up an API to extend what Shell can do. In fact, all command/inputstream methods are built on top of executing Shell.Tasks. Many operations in com.topjohnwu.superuser.io classes are also based on execTask, so it is meant to be very primitive. I let the Throwable thrown in executing the task returned by execTask(Task) instead of thowing it again is because I dislike tons of try {} catch(){}. IMO it makes code more messy. But this is only my personal opinion, I'm not sure if this is against general Java coding standards, and I would love to know your opinion here.

Lastly, returning empty collections is a good idea, this is something I will definitely change. For varargs, I find it easier for me to use the methods, and if you really need to dynamically construct commands, it is fairly easy to construct it with Lists, and use list.toArray(new String[0]) and pass the String array to the method, since Java is smart enough to treat arrays as varargs.

Again, thanks for all the suggestions!

from libsu.

You're welcome! I might not be the best Android developer but I can definitely smell when something looks weird using an API 😉

Design decisions

My library is inspired by Chainfire's libsuperuser. The biggest difference I'm aiming for is a single root shell that is shared by all operations across the app.

About design decisions, it could be nice to add them to the README. It helps a lot to understand where the ideas come from and where the development should go. Specially when you are developing a root application, there is a lot of chance you already know the Jorrit's library.

Output class and callback

The output class (or any better name) seems definitely a good idea to explore.

Java callbacks

About the callback, it feels weird to use an input parameter in Java.
There is some similar concepts in Java:

• java.util.Observable (JDK1) which allows callbacks to observe it to get notified (really useful for UI when Java got released),
• java.util.concurrent.Future (Java 1.5) which allows to be notified when a result is available,
• It evolves with java.util.concurrent.CompletableFuture (Java 1.8) which allows composition but the API is harder to use.
• Observable was deprecated in Java 9 and replaced by java.util.concurrent.Flow which defines standard interface for reactive API. I know Java 9 is not supported in Android but reactive programming has a great adoption within Android ecosystem.
• About Android, android.arch.lifecycle.LiveData could also be a solution. It could be observed and it tends to be the Google way to do it. It also offers a wrapper to reactive API.

Su/sh callback

Let's say we have an Output class which instances are returned from Sync shell.

class Output {
    List<String> out;
    …
}

We could have a subclass or an inherited interface, let's call it LiveOutput, which instance are returned from the Async shell. It should give the same API than Output when the command is finished but also provides API to be notified of updates.

So using those concepts leads to a lot of possible API:

1. LiveOutput could be a LiveData:

class LiveOutput extends MutableLiveData<Output> {
    …
}

// Run command and update the UI on update
LiveOutput liveOutput= su("command")
liveOutput.observe(activity, output -> this.displayOutputs(output.getOut()));
// Just get output lines
List<String> out = liveOutput.get().getOut();

2. Output could contains LiveData:

class LiveOutput {
    LiveData<List<String> err;
    LiveData<List<String> out;
    LiveData<Integer> returnCode;
}

// Run command and update the UI on update
LiveOuput output = su("command");
output.out.observer(activity, this::displayOutputs);
// Just get output lines
List<String> out = output.out.get();

3. Output could be a producer (reactive streams API) of its content:

class LiveOutput implements Producer<String>, Producer<Integer> {
    // We see there is an issue here because Producer<String> is the same for err and out
    List<String> out;
    …
}

// Run command and update the UI on update
LiveOutput output = su("command");
output.subscribe(new Observer() {
    public void onComplete() { }
    public void onError(Throwable throwable) { }
    public void onNext(String out) {
        displayOutput(out);
    }
   public void onSubscribe(Subscription subscription) { }
});
// Just get output lines
List<String> out = output.getOut();

4. Output can offer its own callback mechanism:

class LiveOutput extends Output {
    public void onOutput(Consumer<String> consumer) {
        …
    }
}

// Run command and update the UI on update
LiveOutput output = su("command");
output.onOutput(this::displayOutputs);
// Just get output lines
List<String> out = output.getOut();

My opinion

The solution 1 looks like a bag of everything where any field update will trigger every observer and each observer will need to check if the data they are interested in has changed or not. Quite ugly and inefficient.

The solution 3 is interesting for its reactive approach but fails at some point. There is a lot of boilerplate and there is no interest in onSubscribe and onError callbacks.

So I found the solution 2 and 4 quite interesting.
But The solution 2 needs to add the LiveData library as dependency where The solution 4 needs to define custom interface because Consumer functional interface is not available on old SDK.

(EDIT) Woops

EDIT: I think few minutes later using result to attach callback could lead to miss the first out and err lines.
Let's say the su command runs it immediately in an other thread, we have a trouble here.

So may be a fluent API or builder could be interesting here:

// Run command and update the UI on update
Output output = su(command).onOuput(this::displayOutputs).exec();

// Just get output lines
List<String> out = output.getOut();

So su/sh will no more return an Output instance but a builder, like a CommandBuilder instance which could attach listeners (onOutput(Consummer<String>)) and start the command execution (exec()).

What do you think about this idea?

Shell initializer

I looked at the standard Java API and there is some methods with boolean as return parameter and exception thrown at the same time in the signature. Usually IOException and SecurityException.

Returning an exception means one thing: you should be able to do something about it.
It could be restore your internal state, closing resources, etc… But adding throws Exception means be prepared to recover from trouble. So what could you do when Initializer throws an exception? Actually, nothing else than logging it and do the same thing than when it returns false.

This is why I would not add exception in the signature personally. It's the difference between checked exceptions and runtime exception. If something goes very wrong, the developer will raise a runtime exception (like Integer.parseInt("not_a_number") does) and you could still catch it and log it.

Moreover, throwing the standard Exception without creating dedicated checked exception, like a ShellInitializationException does not help to recover because you can not filter the exception type with different catch blocks. So don't even know you have an initialization exception (may be a NullPointerException or a IOException, etc…).

Tasks & varargs

Hum, I will have a look at it later this week because it is already late here. I will update this post or add a new one if you reply before.

Conclusion

Building an API is hard.
Nobody will find the right way from the first time but a good exercise could be to write (just) the interface of the API and update your code using it accordingly.
If it looks great and fulfills your needs, share it with your users and get a lot of feedback!
If everyone enjoy it (or at least most of them 😇), it is time to think about implementing it.
But implementation should be just a detail at this point 😅

from libsu.

@PerfectSlayer take a look at my current plan for the new API on branch 2.0

Basically, some new classes are defined:

public abstract static class Result {
    public abstract List<String> getOut();
    public abstract List<String> getErr();
    public abstract int getCode();
}

public interface ResultCallback {
    void onResult(Result out);
}

public abstract static class Job {
    public abstract Job to(List<String> stdout);
    public abstract Job to(List<String> stdout, List<String> stderr);
    public abstract Job onResult(ResultCallback cb);
    public abstract Result exec();
    public abstract void enqueue();
}

And some new low level and high level APIs are introduced:

// Low level API
public abstract Job newJob(String... cmds);
public abstract Job newJob(InputStream in);

// High level API
public static Job su(String... commands);
public static Job sh(String... commands);
public static Job su(InputStream in);
public static Job sh(InputStream in);

So to execute commands synchronously and get STDOUT:
Shell.su(cmds...).exec().getOut();

Execute commands synchronously and output STDOUT to specific List:
Shell.su(cmds...).to(List).exec()

Execute commands asynchronously and don't care about output:
Shell.su(cmds...).enqueue()

Execute commands asynchronously and get output after operation is done:
Shell.su(cmds...).onResult(result -> { /* do something */ }).enqueue()

Execute commands asynchronously, reactively get output, get notified when operation is done:

class ReactiveList extends CallbackList<String> {

    ReactiveList(List<String> base) {
        super(base);
    }

    @Override
    public void onAddElement(String s) {
        /* Do something reactively */
    }
}

Shell.su(cmds...)
    .to(new ReactiveList(new ArrayList<>()))
    .onResult(result -> {
        /* Do something to the result that cannot 
         * be done reactively, e.g. get return code */
        result.getCode();
        ....
    })
    .enqueue();

For any developer wanting to do some magic with LiveData or any other stuffs, it is relatively easy to extend CallbackList or ResultCallback to fit their own needs.

Do you have any other suggestions to the new API? I think this API is clean and simple but still very flexible.

P.S. I still create a whole ugly compatibility shim for those still using the old APIs, so the transition will be painless from 1.X.X :)

from libsu.

It really looks nicer to use! 👏
I agree this API is clean and simple. If I can, I would propose some improvements:

• Rename Job.enqueue() to Job.submit().
  enqueue is more related to Collections framework where submit is related to Executors framework

• Rename the two Job.to methods with different method names.
  Ex Job.outputTo(List) and Job.errorTo(List).
  So you could handle error without touching output and you could create inputFrom(List) later for the STDIN.

• Replace the Shell.sh(InputStream) and Shell.su(InputStream) by Job.inputFrom(InputStream).
  So you could remove duplicate sh and su methods and STDIN will be handled like STDOUT and STDERR in the Job class.

So here is the current Job class:

public abstract static class Job {
    public abstract Job to(List<String> stdout);
    public abstract Job to(List<String> stdout, List<String> stderr);
    public abstract Job onResult(ResultCallback cb);
    public abstract Result exec();
    public abstract void enqueue();
}

which could becomes:

public abstract static class Job {
    public abstract Job inputFrom(List<String> stdin);
    public abstract Job inputFrom(InputStream stdin);
    public abstract Job outputTo(List<String> stdout);
    public abstract Job errorTo(List<String> stderr);
    public abstract Job onResult(ResultCallback cb);
    public abstract Result exec();
    public abstract void submit();
}

Even Job.onResult could be removed because it will always be used in combination with Job.summit. So we could overload it:

public abstract static class Job {
    // STDIN, STDOUT, STDERR related
    public abstract Job inputFrom(List<String> stdin);
    public abstract Job inputFrom(InputStream stdin);
    public abstract Job outputTo(List<String> stdout);
    public abstract Job errorTo(List<String> stderr);
    // Execution related
    public abstract Result exec();
    public abstract void submit();
    public abstract void submit(ResultCallback cb);
}

What do you think about this proposals? 😅
Here is the example from your application:

Shell.sh(getResources().openRawResource(R.raw.count))
        .to(consoleList)
        .onResult(new Shell.ResultCallback() {
            @Override
            public void onResult(Shell.Result out) {
                Log.d(ExampleApp.TAG, "async_script_result");
            }
        }).enqueue();

which becomes:

Shell.sh()
        .inputFrom(getResources().openRawResource(R.raw.count))
        .outputTo(consoleList)
        .submit(new Shell.ResultCallback() {
            @Override
            public void onResult(Shell.Result out) {
                Log.d(ExampleApp.TAG, "async_script_result");
            }
        });

One of the main advantage is consistency for the Job class and we can run interactive commands!

Shell.sh("sed 'commands'")
        .inputFrom(getResources().openRawResource(R.raw.somefile))
        .outputTo(editedList)
        .exec();

P.S.: Good luck with compatibility shim! Let's finish the API2 and I could help you with the shim 😉

from libsu.

Honestly, I don't think a compatibility shim would be necessary. Want to use the latest and greatest - migrate your code. Don't want to - remain at the current version and call it a day.

BTW, I'm already thinking about writing RxJava2 wrapper for your new API

from libsu.

Comments

Also, I personally do not like method names to be too verbose (contradict with common Java code naming conventions LOL), so I named my API very simple, like add and to. Also I would love to leave the to(stdout) and to(stdout, stderr) methods untouched too, because in the case of to(stdout),…

No problem. It is you API. I'm just here to challenge your choices to improve the overall API, not to make you change everything!

To only collect STDERR, just use to(null, List).

Hum, null as first parameter is not very elegant. Be sure to add the annotation @Nullable and comment the behavior 😅

API wrappers

About RxJava2 and Architecture components wrappers, do you agree to host own contributions as new modules near to superuser and example? It could offer some visibility and makes them legit (once they are stable enough 😉).
(I guess I can handle the Architecture component part 😄)

Question

Result code

Where you do something like this:

Result result = Shell.newJob().add(InputStream).add(cmds....).add(...).exec();

What contains result.getCode()?
The result of the latest command? Of the first command? What about all intermediate commands, can we get their result codes? Should we have List<Integer> getCodes() and int getLastCode() method instead?

Separate APIs

Could you separate the different APIs? The old one (V1) from the new one (V2) and the high-level API (Shell.su(), Shell.sh) from the low level API (Job, etc…).

The IDE can't tell you which one to use if they all are in the same class.
But if you separate them, high level API will be easy to use (specially with static import) and learn (very few methods).
Moreover, having distinct classes for V1 and V2 wont clutter the IDE outline with a lot of deprecated methods. It will be easier to read and understand 👍

from libsu.

@PerfectSlayer

API Wrappers

Sure, I would love to host them in a separate module if anyone would love to contribute!

Return Code

The current implementation will only get the return code after everything is done (so technically only the last operation in commands or scripts). One job will only have one single return code. I think this is reasonable, because this is how shell scripts works. If you want to get return code for many operations, split them into different Jobs. This extensively in internal.ShellFile, you can check out how I do it myself (it is mostly using ShellUtils.fastCmdResult(...))
In Magisk Manager, I run multiple commands synchronously in the background thread and check the state for each command if needed.

Also a nice trick I used is storing tons of functions in a shell script, and load them into the shell in the Initializer. Then I can call these functions easily afterwards.

APIs

Currently the only "high level" API is Shell.su(...) and Shell.sh(...). I'm not sure if separating these methods is necessary though. The old API is accessed via Shell.Sync and Shell.Async, they are already separated.

from libsu.

@PerfectSlayer, some small adjustments to the API is pushed to 2.0, and all documentation is updated in Shell.java, please check it out.

This should be really close to final. I would have to push this out so I can refactor Magisk Manager, then finally a stable Magisk v17.0 will be release. This library is on my tight development schedule LOL

from libsu.

2.0.0 just released, I'll leave this issue opened for all API discussions in the future 👍

from libsu.

@topjohnwu I'm using Kotlin instead of Java, so it probably won't be looking good in the single project

from libsu.

@victorlapin What is the issue? If you are making a separate module, it should be fine.

@topjohnwu I planed to make the review yesterday night but I had to drive a friend to the hospital… I already start and some some minor bug in your latest commits. So be ready to merge a PR 😅
Whatever, it is nice to see how the API evolve. Good job! 👏

from libsu.

@topjohnwu Do you have any idea about who is using your library?
It could be interesting to gather their feedback and learn which wrapper should be developed first 😉

from libsu.

I am, for example =)
Also, I've seen feedback from the guy who's developing Swift Backup

from libsu.

My library is inspired by Chainfire's libsuperuser. The biggest difference I'm aiming for is a single root shell that is shared by all operations across the app. Most of the design decision is based on my own personal needs in Magisk Manager and cues from libsuperuser.

Both that and to what extent they are compatible might be worth documenting... :)

from libsu.

@Chainfire the "inspiration" part is mostly the su invocation code. A long time ago I have nearly no idea how to code in Java, so I reached out to libsuperuser for creating the first few crappy Magisk Managers. However I need excessive su calls in a short time, and at that time I used Shell.SU in libsuperuser which is far from ideal for that use case. In the marvelous "How-To-SU" documentation, you've mentioned maintaining a long lived root session would be more suitable. So I spent a bunch of time learning Java and implemented my own solution, and eventually I feel comfortable to release it to the public, which was the initial version of libsu.

This library has evolved a lot since the initial release, and I can say that most of the code (other than the part to test root shell) is completely written from scratch. I've not dug deep enough into libsuperuser so I can't say definitely but IMO the 2 libraries are using completely different approaches.

So I would say that the two libraries are completely incompatible LOL, Franco (the developer of Franco Kernel) took quite some effort migrating from libsuperuser to libsu as it is far from a "drop-in" replacement.

from libsu.

Right - from some of the wording above it sounded like it might be an easy migrate, which would be good for people to know (as obviously libsuperuser is not in active development). But it seems that is not actually the case :)

I need excessive su calls in a short time, and at that time I used Shell.SU in libsuperuser which is far from ideal for that use case.

That is a curious statement though, the Shell.Interactive class exists exactly for that purpose, with the ability to add commands and read the results both synchronous and asynchronously, either handling the per-line/total input/output for you, or allowing you to Stream it yourself; even lets you decide what parts to run on which threads.

When used correctly it is very powerful, and certainly fit to run hundreds of commands per second through a single session (or multiple, if you'd like). Then again the documentation and examples are pretty lacking, and it seems many new users have had trouble getting it to do exactly what they want it to, especially in multi-threaded setups.

That's all history though. Again I just had the impression it might be a reasonable drop-in replacement, in which case that should definitely be stated somewhere. As it isn't, it doesn't matter :)

from libsu.

So what about implementing a wrapper to libsu?

I don't know if there is a lot of libsuperuser users who feel to the need to move away from it as it does not seem maintained anymore but providing libsuperuser API using a libsu implementation could be a way to attract some users here. And more users here could lead to a better or longer support of this lib.

I don't know if it worth it?

from libsu.

Actually, I found out Shell.Interactive much later after I developed libsu :), maybe if I found it much earlier I won't have to create this library from scratch LOL

from libsu.