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

Allow running shell scripts from the FileManager #5804

Merged
merged 23 commits into from
Feb 3, 2020
Merged

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Jan 31, 2020

  • Available via the onHold menu, where I stuck it inside the extra separator below cut/delete/rename. Only on shell scripts (.sh).

  • That implies making CRe recognize those (which it will, ideally, read as plain text).

  • Also available in the Favorites onHold menu.

  • Currently runs in the main process, so, blocking, which feels like the right call to me ;). It's bracketed between InfoMessages to make that clear.


This change is Reviewable

This'll have broken the button reordering shenanigans...
Since we're blocking the UI ;).

Also, gratuitous util.template -> T cleanup
Also, use the basename instead of the full path.
What the hell is os.execute doing with that return code o_O.
@NiLuJe
Copy link
Member Author

NiLuJe commented Jan 31, 2020

Currently looks like this:

ko_shell

Any suggestions, @poire-z?

@NiLuJe
Copy link
Member Author

NiLuJe commented Jan 31, 2020

@pazos: Should I disable this on Android?

@NiLuJe
Copy link
Member Author

NiLuJe commented Jan 31, 2020

Also, @poire-z: did I use the BiDi stuff properly?

@NiLuJe
Copy link
Member Author

NiLuJe commented Jan 31, 2020

FWIW, the intended use case is small, simple stuff (say, toggling USBNet, or running a custom sync script (f.g., rclone)).

@Frenzie Frenzie added this to the 2020.02 milestone Jan 31, 2020
frontend/apps/filemanager/filemanager.lua Outdated Show resolved Hide resolved
}

if lfs.attributes(file, "mode") == "file" and string.lower(FrontUtil.getFileNameSuffix(file)) == "sh" then
-- NOTE: We populate the empty separator, in order not to mess with the button reordering code in CoverMenu
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice trick :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I, err, may have lost a few hairs before realizing that I was mangling the nesting in my first attempts >_<".

Comment on lines 316 to 323
UIManager:show(script_is_running_msg)
UIManager:scheduleIn(0.5, function()
local rv = os.execute(util.realpath(file))
UIManager:close(script_is_running_msg)
if rv == 0 then
UIManager:show(InfoMessage:new{
text = _("It exited successfully."),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also use Trapper:dismissablePopen() (like readerdictionary.lua does for sdcv) to get the script output, and allow interrupting it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't actually care about the output (it's sent to the log, which is just fine by me), only the return code ;).

Copy link
Member Author

Choose a reason for hiding this comment

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

But the interruptible aspect might be worth looking into, see discussion below ;).

frontend/apps/filemanager/filemanager.lua Outdated Show resolved Hide resolved
@Frenzie
Copy link
Member

Frenzie commented Jan 31, 2020

Boy, that's really getting crowded.

}

if lfs.attributes(file, "mode") == "file" and string.lower(FrontUtil.getFileNameSuffix(file)) == "sh" then
Copy link
Member

Choose a reason for hiding this comment

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

More of a general remark, but many shellscripts are just "executables" where whether it's a shell script, a python script or even plain old assembly is just an implementation detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'd ideally have just chucked a whole load of extensions in DocumentRegistry, and done the check based on the exec bit, but I can't do that because FAT32 :/.

I'll probably add .py to the list, though ;).

Copy link
Member

Choose a reason for hiding this comment

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

But the only one we know for sure is executable is a Lua script :-P

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm slightly wary of adding lua to the list because of us being full of lua...

frontend/apps/filemanager/filemanager.lua Outdated Show resolved Hide resolved
frontend/apps/filemanager/filemanager.lua Outdated Show resolved Hide resolved
NiLuJe and others added 3 commits January 31, 2020 23:42
FrontUtil -> util
Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>
@pazos
Copy link
Member

pazos commented Jan 31, 2020

Yeeeah! Running shell scripts on root account! What can go wrong? 😃 🙉

You'll need a Python interpreter first, of course ;p.
@pazos
Copy link
Member

pazos commented Jan 31, 2020

Should I disable this on Android?

Yes for this PR. The only problem I see is the ANR, that can be avoided with a dialog on top of the activity (like the animation on 1st launch).

If the script is guarantied to return or is cancellable we're fine. I guess the same happens on other platforms. What happens if the script has a while loop and blocks forever?

@NiLuJe
Copy link
Member Author

NiLuJe commented Jan 31, 2020

If the script is guarantied to return or is cancellable we're fine. I guess the same happens on other platforms. What happens if the script has a while loop and blocks forever?

Then it blocks forever ^^.

The other possibility would be chucking it inside a BaseUtil.runInSubProcess which seems a bit iffy for that use-case?

Otherwise, there's what @poire-z proposed above (Trapper:dismissablePopen) which I'm not terribly familiar with.

@NiLuJe
Copy link
Member Author

NiLuJe commented Jan 31, 2020

Okay, reading Trapper:dismissablePopen, that only works for stuff that outputs stuff to stdout: it can no longer be cancelled as soon as there's output to be read, which wouldn't work here.

@NiLuJe
Copy link
Member Author

NiLuJe commented Jan 31, 2020

That leaves Trapper:dismissableRunInSubprocess, which would probably work but feels a tad overkill here.

I'm obviously biased by being okay with having this blocking the UI, though ;).

@pazos
Copy link
Member

pazos commented Jan 31, 2020

Then it blocks forever ^^.

If this is intended then we can make the same in android, replacing InfoMessages by calls to dialog.show and dialog.dismiss. As in other platforms it will block forever if the script does not return. At least it won't be killed by the activity manager.

I'm not sure if is worth the hassle, though

@poire-z
Copy link
Contributor

poire-z commented Jan 31, 2020

I'm fine whichever way.
(I thought one could build fancy scripts with wget|grep|... and display the output in a TextViewer... but well, whenever I need such fancy stuff, i can patch koreader and have it done in superfancy Lua via some menu item :)

reading Trapper:dismissablePopen, that only works for stuff that outputs stuff to stdout: it can no longer be cancelled as soon as there's output to be read, which wouldn't work here.

You could solve both cases by wrapping the script in something like:

output=`./original_script.sh`; echo "$output" ; echo

@poire-z
Copy link
Contributor

poire-z commented Feb 2, 2020

(minor remark: it will show Execute shell script even on non-shell .py scripts :)

@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 2, 2020

@poire-z: Yeah, but switching to "Execute script" felt wonky to me for some reason (and making the string dynamic depending on the extension seems a wee bit overkill ^^).

@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 2, 2020

Actually, making that label templated is trivial, so, wheee! \o/

NiLuJe and others added 2 commits February 3, 2020 18:29
Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>
Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>
@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 3, 2020

You mean to say I've been using it wrong for all these years? :D.

Latin, man.

@NiLuJe NiLuJe merged commit 5499d85 into koreader:master Feb 3, 2020
@ptrm
Copy link
Contributor

ptrm commented Feb 3, 2020

Wow, I've been just looking for an alternative to not check my dropbox book storage via udev event! (as rclone sometimes redownloads a dozen books even after ntp time sync on kobo).

Thanks 😁

Frenzie added a commit to Frenzie/koreader that referenced this pull request Feb 4, 2020
Frenzie added a commit that referenced this pull request Feb 4, 2020
@pazos
Copy link
Member

pazos commented Feb 5, 2020

Just spent half and hour implementing a longTaskDialog to be used while running scripts. After that I found that internal/external storage are mounted noexec 🤦‍♂️ So nothing to do in android.

@NiLuJe
Copy link
Member Author

NiLuJe commented Feb 6, 2020

@pazos: Does the usual trick of passing the script to the interpreter work, like it does on linux? (i.e., /usr/bin/bash /blah/blah/script.sh).

mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
…5804)

* Allow running Shell/Python scripts from the FM

* Show an InfoMessage before/after running the script

Since we're blocking the UI ;).

* Allow running scripts from the favorites menu, too.
mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
@rigogsilva
Copy link

I saw python in your comments. Does Koreader support run python scripts? (Android 9.0)

@pazos
Copy link
Member

pazos commented Jul 31, 2020

I saw python in your comments. Does Koreader support run python scripts? (Android 9.0)

Python isn't bundled in any platform, but is usually available on some of them. Android is not the case unless you have a very customized device.

Bundling python is out of the scope as the disk space required by the interpreter and the "batteries included" is greater than the entire program, including all fonts for all languages and all translations :)

What could be possible for android is to run python or any other interpreted code using intents. Termux is a pretty powerful environment but, AFAIK, it doesn' allow 3rd party apps to execute things directly, because security reasons.

@rigogsilva
Copy link

Gotcha. I was surprised when I saw that and thought I’d check. Thanks 🙏

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

Successfully merging this pull request may close these issues.

None yet

7 participants