Skeleton commands

The topic was brought up before that, instead of creating wrapper command classes around apps that did their own arg parsing via 'main', classic java style, it would be better if there was a means to simply define the command within the xml descriptor, with no actual command.

==== UPDATE ====
This does not affect the Command class, how it work, or any program that is implemented via the Command interface provide in the shell project. This is simply a means to providing an ArgumentBundle, which is one of the key components required to enable the error handling, completion and help subsystems of the shell/cli interface, for classic java applications. This is to side-step the need for creating a wrapper command class + xml descriptor for each and every class java application.
==== /UPDATE ====

For a first go at this, i want to keep it simple, and I'll make a couple of assumptions.
1) The arguments and associated bundle will need to be instantiated and registered.
2) A command class will _not_ need to be instantiated.

If either of this assumptions is misfounded, then this idea probably will not work as i think it will.

Instead of the command class, we would introduce an extension point that took command definitions. Each command node would map to the 'class' defined in the alias extension point. The child nodes of a command node would be its arguments. Those arguments would take parameters that add any necessary flags or other options that the particular constructor defines. Now this is likely to be the tricky part. It would be fine if the constructor arguments are primitives/Strings, but if a constructor were to take a more complex Object (which i dont think any do at the moment), then this probably would not work. Cases like this may be limited, and could simply fallback on the command wrapper.

==== UPDATE ====
The example below has been updated to show the actual implementation that became of this. Instead of a new extension point, we added a sibling node type, called argument-bundle, that contains a list of argument constructor specifications. These constructor specs are cached in the syntax manager, and at command execution time, get instantiated and collected into an ArgumentBundle for parsing/completing of command lines.
==== /UPDATE ====

To give a clearer picture of what this would look like, lets use Jawk as an example.

<argument-bundle alias="awk">
    <typedefs>
        <typedef name="StringArgument" value="org.jnode.shell.syntax.StringArgument"/>
        <typedef name="FileArgument"   value="org.jnode.shell.syntax.FileArgument"/>
        <typedef name="FlagArgument"   value="org.jnode.shell.syntax.FlagArgument"/>
    </typedefs>
    <argument label="vars" type="StringArgument">
        <param type="flags" value="MULTIPLE"/>
    </argument>
    <argument label="script" type="FileArgument">
        <param type="flags" value="EXISTING"/>
    </argument>
    <argument label="files" type="FileArgument">
        <param type="flags" value="MULTIPLE,EXISTING"/>
    </argument>
    <argument label="compile-dir" type="FileArgument">
        <param type="flags" value="EXISTING"/>
    </argument>
    <argument label="interm-file"  type="FileArgument">
        <param type="flags" value="NONEXISTENT"/>
    </argument>
    <argument label="field-sep"    type="StringArgument"/>
    <argument label="program"      type="StringArgument"/>
    <argument label="interm-out"   type="FlagArgument"/>
    <argument label="compile"      type="FlagArgument"/>
    <argument label="compile-exec" type="FlagArgument"/>
    <argument label="dump-interm"  type="FlagArgument"/>
    <argument label="dump-ast"     type="FlagArgument"/>
    <argument label="xfuncs"       type="FlagArgument"/>
    <argument label="xtypes"       type="FlagArgument"/>
    <argument label="sort-arrays"  type="FlagArgument"/>
    <argument label="no-fmt-trap"  type="FlagArgument"/>
</argument-bundle>

Instead of having the alias manager lookup the class for that alias, it would look up the command descriptor, create an argument bundle, and register the defined arguments. I still have yet to dive into particular details, but this is the idea i've had so far. If it doesn't seem to far off track, i'll look deeper into what would actually be needed to make it happen.

==== UPDATE ====
This actually wound up having no affect on the alias manager. Everything still works the same in that regard.
==== /UPDATE ====

Misundersanding

(In response to mumitr0ll)

I think your misunderstanding what this patch is doing. It's not taking anything away from commands that are coded by extending AbstractCommand, in fact it does nothing to affect them whatsoever. Instead, it is bringing the same features that a command extending AbstractCommand would have to applications that use the classic java main() entry point. The same thing could be done if you were to wrap every single application with a class that extended AbstractCommand, and invoking main() itself. We wanted to get away from having to create these wrapper classes, but still be able to provide the features that AbstractCommand brings via the ArgumentBundle.

Again, this does nothing to affect people that wish to work on commands programmed as an extension of the Command interface. It simply makes it easier to bring classic java applications into the loop, without adding any new java code.

I hope that clears things up.

Supply patches

Hi,

besides what cluster said, I want to note some other things.
I remember you said on IRC a while back that you are confused by the second repository (git) and that you still miss some documentation parts for your commands. I highly suggest that you create a issue for the command you're currently implementing and assign yourself to it. So you don't have the risk someone else is going to implement it first. Secondly you could supply patches to that issue so we can review it, help you and give comments if you wish that.

Besides that, the git repository (explained in the documentation) gives everyone the possibility to "commit" their stuff to a mirror of the official svn repository. So everyone can commit to that and it's easy to track those changes. Once your code is good enough, someone with svn commit can commit it back. But anyway, if you feel a need of documentation for something like that, please create an issue or go ask on the forum. E.g. I thought the git documentation was well enough, but only if guys like you tell us, that something is missing for your understanding, we can improve on it.

So in that sense, I hope to see many "issues" from you from now on Smiling

The following changes since

 distr/descriptors/org.jawk.xml                     |   81 ++++++---
 distr/src/apps/org/jawk/JawkCommand.java           |  104 -----------
 distr/src/apps/org/jawk/JawkMain.java              |   50 ++++++
 shell/src/shell/org/jnode/shell/CommandInfo.java   |   53 +++++-
 shell/src/shell/org/jnode/shell/CommandRunner.java |    6 +-
 shell/src/shell/org/jnode/shell/CommandShell.java  |   20 +--
 .../org/jnode/shell/syntax/ArgumentSpecLoader.java |  185 ++++++++++++++++++++
 .../jnode/shell/syntax/DefaultSyntaxManager.java   |   58 ++++++-
 .../org/jnode/shell/syntax/SyntaxManager.java      |   19 ++-
 .../org/jnode/shell/syntax/SyntaxSpecLoader.java   |    4 -
 10 files changed, 422 insertions(+), 158 deletions(-)

Color Diff
Plain Diff

wget "http://repo.or.cz/w/jnode-mirror.git?a=commitdiff_plain;h=4bb191491f47bcb8399f43f7191f2e0ab8621462" -O - | patch -Ep1

One thing to note, if your patcher does not get rid of empty files, you'll have to rm distr/src/apps/org/jawk/JawkCommand.java. patch -E would do the trick

This works for execution. If the syntax parser fails, then it wont execute the main() method.

Completion isn't working yet though, I'm not sure where this has to wire in. The ArgumentBundle is getting created, but something is preventing the completion from going through, will debug it more in a bit.

I will be making a couple of

I will be making a couple of adjustments to this patch, nothing logical, just grammatical. I'll change the tags from args and arg to argument-bundle and argument, for clarity.

Also, I have everything more properly documented in a follow-up commit. I gave a class doc to the parser to explain the syntax and can be used for a concise syntax reference. I've also amended/added javadoc where appropriate to explain a methods role in helping bare commands where necessary.

As for the syntax command, i have taken a look at what would be needed to include this, and it would be simple enough, but i wasn't sure how far the syntax command was going to be taken. Obviously it makes for a nice command for a developer working on a syntax, but its also a security risk, and probably wouldn't see the light of day in a production system (at least not without safeguards/elevated privileges). Having said that, i will likely add this to its capabilities, though i'll wait until this patch has settled in.

I hope you mean "argumentBundle"

... "argument_bundle" would violate the JNode house style rules. Apart from that, it looks good.

actually i meant it with a

actually i meant it with a -(hyphen) not _(underscore). I wouldn't have thought the java source rules would extend into xml tag names, but ok, <argumentBundle> it is then.

I'll put together a commit and check it in a bit later.

Sorry ... my mistake. I

Sorry ... my mistake. I thought you were talking about Java.

A '-' is right for XML.

completion

 shell/src/shell/org/jnode/shell/CommandLine.java |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Color Diff
Plain Diff

wget "http://repo.or.cz/w/jnode-mirror.git?a=commitdiff_plain;h=fb4c88fd1b98524b7ba25c5fd70d7302989d6ca3" -O - | patch -p1

Good idea

... lets do it Smiling

Ok, so before i get started,

Ok, so before i get started, i'd like to clear a couple of things up.

Should i rework the AliasManager to track the command EPs or should there be a CommandManager also?

And if we use the command manager approach, do we reclassify org.jnode.shell.command for this purpose?

Lastly for now, would a new constructor of the form Alias(String,String) be ok for creating these types of aliases? I was thinking to add an isBare() method to Alias to determine thats its 'className' does not refer to a Class<?>, but instead its a bare command defined in the command manager for which the caller can query to obtain the argument bundle for that command.

I'm going to get started assuming the answers are > Command Manager > Yes and Yes,

As discussed in IRC with

As discussed in IRC with levente, the command manager/ep idea was squashed in favor of creating a sibling node to the syntax ep. So this would mean No and No to my first two questions. Instead, the SyntaxManager can gain a getArgumentBundle() method for an alias that is not a class of type AbstractCommand. If a bundle is returned, then we can use the bundle for completion, and ultimately for checking the command line against the syntax.

Ok, i have the spec parser

Ok, i have the spec parser part done, creating and caching an ArgumentBundle. (I know this has to change)

The problem i have now, is where to tie in the bundle so that it parses/populates the arguments, i had thought CommandInfo.parseCommandLine would be the spot, but I think that might be too late. Any hints on where I need to 'subvert' the execution process and force it to run the parser before calling main()?

And just so you have an idea of what i've done. The SyntaxManager is returning the cached bundle for me. I know i can't really cache and use the same bundle over and over, i'll change that once i get it working. Am I going to need to create an anonymous AbstractCommand for this? or do you think i can get away with no Command class at all?

For your amusement...
http://repo.or.cz/w/jnode-mirror.git?a=commit;h=2f7dcda20cd873f2279ed06a...

Wiring and using the argument bundle

If you want to fit in with the way the way that the syntax specs are currently handled, I suggest:

  • add a new attribute (with getter and setter) to CommandInfo with a suitable name,
  • modify CommandShell.getCommandInfo to lookup the argument bundle spec and populate the new attribute
  • modify CommandInfo.parseCommandLine to handle the case where there is an argument bundle spec.

I don't think you need to arrange that a Command object is created. With a "real" JNode command class, the AbstractCommand layer is used to register the Arguments and create the bundle, provide the CommandIO objects and to support the hack that allows a JNode command to be entered via the main(String[]) method and still use the JNode syntax mechanisms. A class Java command requires none of these, so an actual Command instance should be unnecessary.

this answer seems to negate the point

I thought the point was to cache the commands. How does this suggestion for tying in the bundle here help?

Again, I've been looking at the commands in detail. So I'm following this discussion.

-Cluster- do you have new thoughts on how to do the command cache?

Dereck

I dont think we really want

I dont think we really want to (or for that matter, that we even can) cache the Command object. And really it doesn't make any sense to. Perhaps this isn't quite what you meant and I'm misinterpreting?

Tying in the bundle like this means we get a fresh ArgumentBundle on each invocation. Until proven otherwise, caching in this context seems like a premature optimization.

You cannot cache command objects

This is because, the process of parsing command arguments modifies the Arguments in a command object's argument bundle. Not to mention that the execute method will modify the command object's state.

The non-reuse of command objects was a deliberate design decision. Trying to reuse them leads to all sorts of problems. Its just not worth it.

Then the point is...

only to use a different XML based syntax mapping the commands? I guess I'm not sure why we are doing this particular refactoring if we aren't gaining efficiency.

I'm not sure that a categorial "you can not cache commands" is correct. But no matter, I haven't time to debate this.

You don't have to respond. I'll wait until the refactoring is complete. I guess I'm not sure what this is gaining us. It _does_ mean that people working on commands actively have to wait until things shake out before work can be submitted. I understand that the old commands will still work, but what is the point of creating new commands in the old style? When this is done I'll relearn how to add commands.

I will add, though, that in the Enterprise Java space there is a strong tendency to move away from XML configuration for everything due to its tediousness. If I've heard it once I've heard it a thousand times that the "XML configs multiply!" The old AbstractCommand/Command invocations were simple enough to understand. If we aren't going to be gaining a cache or some other efficiency in doing this, is it just a 'look and feel' of the code aspect toward using this XML descriptor system?

Again, no need to respond. I'll wait for these things to shake out.

OK ... the headline is an exaggeration

I'm not sure that a categorial "you can not cache commands" is correct.

You are right. If we reset the state of the arguments in the argument bundle and made it a rule that a command class instance had to reset all state on completion of the 'execute' methods ... we could in theory cache them.

But IMO that would be a bad idea. We cannot rely on every developer remembering to follow the rule. And if someone forgets to follow (or deliberately ignores) the rule, commands may behave differently each time the user runs them, in ways that are hard to fathom. Worse still, a command could conceivably leak private information via its instance variables, by accident or by design. (On the last point, there are other ways that a command class can leak information ... but it is still not a good idea to create more "holes".)

The other point is that we have a goal that commands would "normally" be executed in clean new isolates. But when you run a command in a separate isolate, the command class and its argument objects need to be instantiated in the child isolate, not the parent isolate. (See discussion on the tracking issue for isolates.) So unless you cache and recycle isolates (and the implications of that are really scarey!) you end up having to create new command class instances whenever the "isolate invoker" is used to run a command.

Finally, we need to keep this in perspective. The cost of instantiating a typical command class is not that large, especially compared with the other things that have to happen to run a command; parsing the shell command line, parsing the command arguments, creating a new thread/proclet/isolate, managing the standard streams ... and running the command. And of course, the first time you run a command, JNode needs to load and compile the command class and its dependencies. Now that can be expensive!

Anyway. When we are "all done and dusted", and we have the time (and inclination) to speed up command launching, then we can crank up a profiler and loot ar where most of the time is being spent. That may say that command instance caching would be a worthwhile thing to try. But right now, I think it is a premature optimization with doubtful advantages and (IMO) clear disadvantages.

Get it right first, then make it fast.

I got my money on the

I got my money on the compiler, not the code.

There is absolutely no doubt ...

... in my mind that the JNode compiler is what causes many commands to take a long time the first time you run them.

  1. The JNode compiler is not a true JIT compiler. Specifically, when you call Class.forName(), the compiler compiles all classes in the static dependency graph for the loaded class. By contrast, a true JIT compiler would compile code (methods) only when its (they) are actually needed (called). Indeed, Sun's Hotspot compiler uses the bytecode interpreter to execute methods the first few times to gather profiling information. This means that methods that are rarely called (as well as those that are never called) are not compiled at all.
  2. The JNode compiler does not produce particularly good native code. Since this compiler is used to compile itself (as part of the JNode build process) the compiler will be slower than it could be.
  3. The JNode compiler may not be coded to be as efficient as it could be. If that was true, it would impact on compiler speed, and slow down command loading.

I suspect that the first point is the most significant cause of command load/compile slowness.

I think your

(reply is in the post at the top of the page, the forum keeps squashing replies smaller and smaller, and they're getting unreadable...)