FindCommand implementation

The exception is telling you that the parser has encountered a situation where the arguments have matched the syntax without binding an actual argument to the "path" Argument. In other words, your syntax specification is inconsistent with what FindCommand has said it needs. In this case, the problem is in your syntax specification:

   <syntax alias="find">
      <empty description="find files and directories"/>
      <argument argLabel="path" description="directory from which to start searching"/>
      <argument argLabel="search-string" description="search string to use"/>
      <optionSet>
          <option argLabel="size-above" shortName="S" longName="size-above" description=""/>
          <option argLabel="size-above" shortName="s" longName="size-below-equal" description=""/>
          <option argLabel="dirs-only" shortName="d" longName="dirs-only" description=""/>
          <option argLabel="files-only" shortName="f" longName="files-only" description=""/>
          <option argLabel="exact" shortName="e" longName="exact" description=""/>
          <option argLabel="invert" shortName="i" longName="exact" description=""/>
      </optionSet>
   </syntax>

This says that the expected arguments are nothing, or a "path" or a "search-string" or a one or more options; i.e. four separate syntax variants. The child nodes of a <syntax> node are an implicit "alternation". (Perhaps this should be more clearly stated in the documentation. But certainly the "cat" example shows this.)

I think you probably mean the syntax to be something more like this:

   <syntax alias="find">
     <sequence description="find files and directories">
      <argument argLabel="path" description="directory from which to start searching"/>
      <argument argLabel="search-string" description="search string to use"/>
      <optionSet>
          <option argLabel="size-above" shortName="S" longName="size-above" description=""/>
          <option argLabel="size-above" shortName="s" longName="size-below-equal" description=""/>
          <option argLabel="dirs-only" shortName="d" longName="dirs-only" description=""/>
          <option argLabel="files-only" shortName="f" longName="files-only" description=""/>
          <option argLabel="exact" shortName="e" longName="exact" description=""/>
          <option argLabel="invert" shortName="i" longName="exact" description=""/>
      </optionSet>
     </sequence>
   </syntax>

... which declares a single syntax variant consisting of a "path" followed by a "search-string" followed by options.

One way to debug these things is to run "help find" and look carefully at the "usage" information output. Assuming that there are no bugs in the core shell codebase, the "help" output should correspond to how the argument parser understands the syntax specification.

BTW: the argument syntax you have designed so far is nothing like the POSIX "find" command. That's OK, but if you are going to continue with an incompatible design, I think that your command should not be called "find". At some point, someone is going to want to create a POSIX compatible "find" for JNode ...

BTW 2: you shouldn't need to use the Bjorne interpreter for your command to work. Though it is gratifying to have someone using it

The find command syntax is not very intuitive. I thought it would be a good idea to simplify it a bit.
But if you guys think the syntax should really be the same i can do it this way.

EDIT:
I changed the syntax specification:
Still doesnt work...

BTW: how can i post XML code?

I added the possibility to upload xml files.

Regarding the command's name I have the same opinion as Steve: Either mimic the Linux-find-syntax or rename the command to something else.

A comment to the code: We have coding conventions for some time and we have a checkstyle file for it. One thing that I recognized without deeper looking was, that you used tabs instead of spaces at some places.

Regarding the syntax: I guess the optionset should not be a new syntax but you want it in the same sequence as the other options. Currently "find _path_ _search_string_" and "find -f" both are valid syntax but "find _path_ _search_string_ -f" is not. Without having tested it, I'd say the optionSet should be within the sequence.

Ok, it now works!
At least I can call "help find" and get correct parameters and options.
But when calling "find / jnode" i get an Security Exception concerning threads...

Regarding code style: Where can i find the coding conventions?

Do's and Dont's can be found here, the coding conventions for JNode are here. As the document states, you can run the rules with "./build.sh checkstyle".

I'm not sure why you get a Security Exception in that case but for testing you might simply assign all permissions to your command (Though that should not be checked in but makes testing easier).

  <extension point="org.jnode.security.permissions">
        <permission class="java.security.AllPermission" />
  </extension>

You asked "how can I post XML". Peter has changed the website to allow you to attach XML files comments, etc. But if you wanted to put XML into the text of a comment (like Peter and I have done above), you should put <pre> ... </pre> tags around the XML block, and then go through the XML replacing all '<' characters with "&lt;" sequences.

Concerning XML in the comment text: i tried it with pre and /pre

<pre>...</pre>

the result was only empty lines...(just like in this case...)
I reformatted classes to fit coding style conventions. Now I get following error:
"Error getting help for alias / class 'find': NPE at adress 1508...."

Sorry, I was assuming that you were familiar with HTML. The <pre> and </pre> tags must each be on a line by itself. Your comment should be entered something like this:

    ... yadda yadda, and here is some XML:
    <pre>
        &lt;some-xml-tag&gt;
            ...
        &lt;/some-xml-tag&gt;
    </pre>

which should render as:

... yadda yadda, and here is some XML:

    <some-xml-tag>
        ...
    </some-xml-tag>

... "Error getting help for alias / class 'find': NPE at adress 1508...."

Stack trace please. A screen shot would be fine.

is there a way to scroll through console output or enlarge the console?
stack trace does not fit on current screen...

Console key bindings are described in the JNode Shell page. There are key bindings for scrolling a console. I don't think it is possible to resize the JNode console.

Your stacktrace tells me that the NPE is happening in your code when Help is creating the instance of your command class in order to find out what Arguments are registered. Unfortunately, it is not possible to be more specific. There is a long standing bug in JNode that means that you don't see the top stack frame following an NPE. If you cannot make progress by eye-balling your code, you will need to resort to adding traceprints to the constructor (I guess) to figure out where the NPE is being thrown.

i found the problem:

 PrintWriter out = getOutput().getPrintWriter(); 

returns null. But what is wrong with that?

KeyBindungs concerning arrow keys do now work for me. (Thinkpad Z61m, german keyboard).

Why are you calling that in the constructor? A command's streams have not been set up when the constructor is called. You should only call getOutput() etc from the execute method.

...Why are you calling that in the constructor? A command's streams have not been set up when the constructor is called.
I did not know that...
fixed it. So I'm back to the security error...

Does this have something to do with it?

private final ExecutorService ex = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() * 3);

Um ... maybe.

Looking at the stacktrace, the problem seems to be that AtomicInteger is doing something internally that violates the default security rules. This deserves its own issue. Please create one, including a simple test case if possible.

But, your question suggests that you are trying to use ExecutionService, and I don't think this is a good idea in a "find" utility. It will make the output of the command non-deterministic, and in a multi-user environment it will be a resource hog. If you really want to implement a fast file find there are better ways to do it than parallel searching. OTOH, a parallel file searcher might uncover some concurrency bugs in the FS tree ... so perhaps I should encourage you to continue

To make progress on your command, do as Peter said / described above and add an permission entry to the plugin descriptor to (temporarily) allow the operation.

I've updated AbstractCommand.getOutput() and related methods to check that 'initialize' has been called. The method will now throw an IllegalStateException rather than a NullPointerException if used incorrectly.

But, your question suggests that you are trying to use ExecutionService, and I don't think this is a good idea in a "find" utility. It will make the output of the command non-deterministic, and in a multi-user environment it will be a resource hog. If you really want to implement a fast file find there are better ways to do it than parallel searching.

A simple recursive implementation is out of question. It comes quickly to a stackoverflow.
I could also use a SingleThreadExecutorService but I see no reason to not use multiple CPU's on modern multi-CPU systems...

If you are worried about stack overflow, implement your own stack data structure and make your code non-recursive.

... but I see no reason to not use multiple CPU's on modern multi-CPU systems

Don't you see a problem with having your command output the files in a different sequence each time?

But hey ... implement it how you want to. If it turns out to be a bad idea, someone else can always change it later on.

Don't you see a problem with having your command output the files in a different sequence each time?
no, not really.
With unix find it is also a bad idea to rely on the order of output.
File content can always change.
But if you guys dont like it, I change it.

Could you please submit the code for a review?
That might be a more efficient solution for clearing the issues.

Could you please submit your current code again for a review?
That might be a more efficient solution for clearing the issues.

Could you please submit your current code again for a review?
That might be a more efficient solution for clearing the issues.

I'm right now finishing a single threadded implementation, that does not work recursively (output is just like the one from unix find).
As soon as its finished, I will post the patch again.

Hello there,

I'm trying to test my current find implementation but unfortunately the jnode startup gets stuck with 100%CPU at some point.

I attach screenshot and current patch for the find command.

Please include in the patch the XML descriptor (alias & syntax) of the command too.

Ups... here it is!

This one should work now...

I believe that Levente has committed this patch. Should the issue remain active? Are you continuing to work on this Kasper?

Improvements are in progress.

Here is another patch:
changes:

• added short option identifier to options
• added description strings
• fixed security exception issue
• directory argument is now optional, if not given, working dir is searched. unfortunately this brings another bug, which is that if you provide a directory to search, this is ignored and working dir is searched (without filtering). Cannot tell why that is, currently...

Attachment	Size
findCommand4.patch	8.3 KB

another....

I have committed your patch because it has improvements compared to the previous version but futher work is required.

I think the provided directory argument is being ignored for some reason and this is why always the current directory is searched. You can also see that there is no more directory name completion either.
I suspect a bug in the shell command infrastructure as the cause of this.

The syntax originally was:

    <syntax alias="find">
      <sequence description="find files or directories">
        <repeat minCount="1">
          <argument argLabel="directory" description="directory to start searching from"/>
        </repeat>
        <optionSet>
          <option argLabel="type" longName="type" shortName="t" description="filter results to show only files of given type. valid types are 'd' for directory and 'f' for file"/>
          <option argLabel="maxdepth" longName="maxdepth" shortName="D" description="descent at most to given level of directories"/>
          <option argLabel="mindepth" longName="mindepth" shortName="d" description="ignore files and directories at levels less than given level"/>
          <option argLabel="name" longName="name" shortName="n" description="filter results to show only files that match given pattern"/>
          <option argLabel="iname" longName="iname" shortName="N" description="same like 'n', but case insensitive"/>
        </optionSet>
      </sequence>
    </syntax>

Then you changed the: repeat minCount="1" to repeat minCount="0" and the code completion stopped working was well as the argument seems to be ignored since then. This is a too dramatic behavioural change for a simple changing of an argument value from 1 to 0.
Steve, could you please take a look at this issue and see if it's a bug in the command syntax infrastructure or a problem in the sytax specification of the find command?

I'll take a look tonight.

Update: it looks like there is a bug in the guts of the MuParser code. I'm trying to track it down.

I believe I've found the problem. A patch is attached to this comment if you want to try it out.

I want to do some more thorough testing before I check in this fix ...

I've now committed the fix. FYI, the problem was that there was a situation where MuParser would "succeed" when there were unconsumed tokens (arguments).

The current syntax works, but there is a flaw in it. For instance "find . -name foo bar" will surprisingly bind 4 values to the "dir" argument. This happens because "-name" is not an option name in the JNode find syntax but it is actually a valid JNode filename. A workaround (in this use case) is to change FindCommand to add the "must exist" flag to the flags passed to the FileArgument constructor. But a more general solution is required. I'll add this to the list.

(Updated example above ... to correct typos that destroyed the meaning.)

Hmm, just curious but shouldn't it be "\-\-name" for a filename?
Though I just tried it in bash and honestly, I didn't find out how to touch a file named "--test" or mv,cp,.. a file named "--test" (had to use mc to create the file at all ).

touch -- --test

-- is used to say 'stop parsing options'

I added the "Argument.EXISTING" flag to the file Argument, but it still seems impossible to pass any directories to the command. "find . --name foo" still does no filtering.
Auto Completion also does not work for file arguments...

I don't get it. My sandbox is now in sync with the HEAD revision. The 'find' command is accepting directory arguments just fine for me; e.g. 'find jnode/tmp' outputs just "jnode/tmp". And 'find / --iname tmp' shows it as well. Also completion is working just fine for me for the 'find' directory argument(s), and for other commands as well.

I suggest that you temporarily put your local mods to FindCommand, etc on one side, sync up to the HEAD revision and see if the problems you are reporting go away.

Something else too. Find shouldn't be using Pattern directly for -name and -iname. There's org.jnode.shell.PathnamePattern that can compile shell wildcards into a Pattern. I wouldnt remove what you have for -name and -iname though, just convert them to -regex and -iregex, as Pattern is fine for these tests for now.

Would also be nice to be able to add multiple tests of the same type. I know we can't do the boolean tests properly yet, but at least we can use the implied -and between two tests like -find . -name "a" -name "b"

The output on stdout shouldn't be absolute pathnames. The current directory prefix should be stripped. Would also be nice to be able to -print0 to output null terminated files instead of newline terminated.

The output on stdout shouldn't be absolute pathnames.

Are you sure? Try running 'find /tmp' on Linux.

Let me rephrase.

The output should not be absolute pathnames unless the initially supplied directory isAbsolute(). Otherwise the output should be relative to the current working directory.

Also there is a bug with . and .., but perhaps this is more of a bug with java.io.File. The problem is, . and .. return a directory that has a following slash, this is a problem later because calling listFiles() on that directory outputs a name with a //. For example, from /jnode/tmp with a directory 'test' and a file 'a', i got this for f.toString() and f.getCanonicalPath()

/jnode/tmp//test  # f.toString()
/jnode/tmp/test   # f.getCanonicalPath()
/jnode/tmp//a
/jnode/tmp/a
And hence why no files other than /jnode/tmp/ are returned from find .  

Ok, updating to head and clean build solved the problem.
seems to work now.

Here is a patch that brings java doc.

I think you have a typo in AbstractDirectoryWalker

public void stoppWalking() {

Guessing that should be stopWalking.

right!

I commited your javadoc patch with fixed typo, and i added a couple of changes to ADW.

I removed the getCanonicalFile() calls. If the caller wishes to have ADW use canonical pathnames, than it should make sure they are canonical before calling walk(). Its perfectly acceptable in many situations to have ../dir or even /dir1/dir2/../dir3.

Also i added a walk(List) method for convenience.

I think if you remove the first toCannonicalFile() call, "." and ".." as file arguments are no longer handled right....

I think they're handled propery. If i type in find ../dir in linux, i get a bunch of listings with ../dir/somewhere in my output. and thats what it outputs now. Same with find . should output ./

Besides, if the caller wants canonical form, they can pass canonical files. I just dont think its right that ADW forces canonical form, as its not always wanted.

I just dont think its right that ADW forces canonical form, as its not always wanted.

You are absolutely right cluster.

If we are serious about being able to run POSIX shell scripts JNode, it is essential that the JNode versions of common utilities behave as POSIX specifies.

Can I request that you create the User Command page for 'find' before closing this issue. Please use the source markup for one of existing command pages as a template, and try to conform to the same style and structure.

What is a "user command page"?

See http://www.jnode.org/node/2053 for user command pages.