Added "pcbignore" to ignore newlib dirs/files

[Jeff Mallatt]

In my private newlib style libraries I have some non-footprint files, which were showing up in the Library window. I noticed in the code (in file.c) that a few directories/files are hard-coded as to be ignored. I figured that adding a user-configurable mechanism to ignore dirs/files would be more useful to others than just adding my names to the hard-coded list.

So I implemented a simple "pcbignore" mechanism. Allows for per-user and per-newlib ignore lists. The patch adds an appendix to the doc/pcb.texi file (q.v. for details on use).

[Bert Timmerman (bert-timmerman)]

Hi Jeff,

Good idea ;-)

I have tested this patch in my local git repo <https://github.com/bert/pcb/tree/LP1005644>.

Copying .gitignore to .pcbignore in the toplevel pcb dir and the newlib
dir doesn't seem to give the expected results.

For example, when choosing "File"->"Open" still shows the Makefile and
Makefile.in etc. in both above mentioned dirs.

Also for choosing "File"->"Load element to buffer".

I have verified in config.h that:

<quote>
/* Define to 1 if you have the `fnmatch' function. */
#define HAVE_FNMATCH 1
</quote>

is set so that no "hard-coded internal fakery" is performed.

Maybe I have misunderstood the documentation or maybe the patch is not complete.

And maybe add a more extensive explanation of these rules in the
documentation.

For example, ".gitignore" files have the same functionality for git
and are parsed according following pattern formats, and the rules
are described in "man gitignore" as:

- A blank line matches no files, so it can serve as a separator for
  readability.

- A line starting with # serves as a comment.

- An optional prefix ! which negates the pattern; any matching file
  excluded by a previous pattern will become included again.
  If a negated pattern matches, this will override lower precedence
  patterns sources.

- If the pattern ends with a slash, it is removed for the purpose of
  the following description, but it would only find a match with a
  directory. In other words, foo/ will match a directory foo and paths
  underneath it, but will not match a regular file or a symbolic link
  foo (this is consistent with the way how pathspec works in general
  in git).

- If the pattern does not contain a slash /, git treats it as a shell
  glob pattern and checks for a match against the pathname relative to
  the location of the .gitignore file (relative to the toplevel of the
  work tree if not from a .gitignore file).

- Otherwise, git treats the pattern as a shell glob suitable for
  consumption by fnmatch(3) with the FNM_PATHNAME flag: wildcards in
  the pattern will not match a / in the pathname.
  For example, "Documentation/*.html" matches "Documentation/git.html"
  but not "Documentation/ppc/ppc.html" or
  "tools/perf/Documentation/perf.html".

- A leading slash matches the beginning of the pathname.
  For example, "/*.c" matches "cat-file.c" but not
  "mozilla-sha1/sha1.c".

Kind regards,

Bert Timmerman.

[Jeff Mallatt (jjm-j)]

At 2012-08-06 07:07, you wrote:
>Hi Jeff,
>
>Good idea ;-)

Thanks.

>I have tested this patch in my local git repo
><https://github.com/bert/pcb/tree/LP1005644>.

And thanks for testing the patch!

Also, sorry for not responding sooner -- my development environment blew up, and I needed to re-construct it before I could refresh my memory and make sure things were working as I thought.

>Copying .gitignore to .pcbignore in the toplevel pcb dir and the newlib
>dir doesn't seem to give the expected results.

The format and meaning of pcbignore files is different from gitignore or cvsignore files. It only understands three things: "!" to clear the list; filename globs (e.g. "*.foo", "GNUmakefile"); and directory names (marked by leading "/"; only applicable to the root directory of newlib-style directory trees; e.g. "/unreleased").

Also the location(s) where it is looked for is more restricted.

A pcbignore file in "~/.pcb/pcbignore" applies equally to every newlib-style root directory and their immediate subdirectories. Then each of those newlib-style root directories and subdirectories may have a ".pcbignore" file in them, and that ".pcbignore" applies to only that directory or subdirectory. There is no "searching up the tree"... At least, not yet -- I now think that this would be a good idea, and I'll add it to the next version of the patch.

>For example, when choosing "File"->"Open" still shows the Makefile and
>Makefile.in etc. in both above mentioned dirs.
>
>Also for choosing "File"->"Load element to buffer".

And pcbignore only applies to the newlib "PCB Library" Window.

I don't really know much about this aspect of the PCB code, but when it comes to File menu operations, doesn't that depend upon which HID is used? That's in a very different area of the code. E.g. it looks like the GTK HID would need src/hid/gtk/gui-dialog.c to be modified, and then it could do only those things that the GTK library could do. This is another area that could be looked into, but I'm not sure that the pcbignore idea would work here.

>I have verified in config.h that:
>
><quote>
>/* Define to 1 if you have the `fnmatch' function. */
>#define HAVE_FNMATCH 1
></quote>
>
>is set so that no "hard-coded internal fakery" is performed.

Okay, good.

>Maybe I have misunderstood the documentation or maybe the patch is not
>complete.

The patch is complete in that it does what I intended. That does not mean that the patch can't be extended to do more than it currently does. :-)

>And maybe add a more extensive explanation of these rules in the
>documentation.

No doubt that is always true. :-)

I'll extend it in the next version of the patch.

So I'll add searching up the directory tree for .pcbignore files, and I'll try to improve/extend the documentation, and then re-post a patch to Launchpad, and send a message to the geda-user mailing list. Should be within a few days. If you have any other comments or suggestions before then, just let me know.

Thanks!

[Jeff Mallatt (jjm-j)]

Thanks to great feedback from Bert Timmerman, I have updated my pcbignore patch.

Added functionality, making pcbignore files more flexible. Also greatly improved the
documentation.

Removed the old patch, and attached a new one (this is against the latest repository).

[Bert Timmerman (bert-timmerman)]

Hi Jeff,

Thanks for the updated patch.

I applied the patch to my development repository and did a superficial review.

AFAICT no build problems encountered.

I noticed some indentation details, see my comments at:

https://github.com/bert/pcb/commit/b6e793f47e883da7c3bed214581fcb08d27f91db

Will further test and look into this patch starting Thursday 30th.

Kind regards,

Bert Timmerman.

[Bert Timmerman (bert-timmerman)]

Oh, forgot to mention this one lives on a new branch:

https://github.com/bert/pcb/tree/LP1005644rev1

Kind regards,

Bert Timmermn.

[Jeff Mallatt (jjm-j)]

Thanks for looking at the patch.

As for indentation, I've been simply using emacs C-mode, which uses as many 8-column-wide tabs as possible, followed by spaces to the indentation position. When I looked around at the file, that seemed to be what was mostly used already. When displayed with 8-column tabs, the indentations all look correct to me, and appear to largely conform to the default indent program.

If, however, you want a different method -- all spaces, for instance -- just let me know.

Ah, I just noticed something on this commit page:

    https://github.com/bert/pcb/commit/b6e793f47e883da7c3bed214581fcb08d27f91db

when I click on the "View file @ b6e793f" button for src/file.c, the indentation as displayed on that page appears to be correct. Is it possible that the diff displayed on the commit page collapses tabs into single spaces or something?

[DJ Delorie (djdelorie)]

> As for indentation, I've been simply using emacs C-mode, which uses
> as many 8-column-wide tabs as possible, followed by spaces to the
> indentation position.

That's the right mode to use, yes. I use that too.

[Jeff Mallatt (jjm-j)]

Just checked to make sure the patch still applies after
all the recent commits. It does, though with some line
number variances.

Also have looked again at indentation, and it appears to
me to be correct. Please let me know if I'm not seeing
something.

Thanks.

[Peter Clifton (pcjc2)]

Does the updated version of this patch work closer inline with the .gitignore .cvsignore style?

Given Bert's intially assumption (and mine), that this is the kind of syntax which should be applied - I'd be strongly inclined to favour that syntax and semantics.

However.. I'm not entirely convinced why we need this.

Would the problem you're trying to address also be solved if we could mandate that only "*.fp" files are displayed? (Or if PCB could skim the header of the file to determine if it actually _IS_ a footprint).

Forcing *.fp files in libraries would be is a non-trivial change, as we never have insisted on a particular file extension in the past, but one I'd be prepared to discuss making. The complexity of that change is overall low, compared to adding a mecahnism for blocking arbitrary files.

[Peter Clifton (pcjc2)]

Peter B: Could I get you're opinion on this, given you just recently fixed a similar sounding issue for gEDA, and on the general "design philosophy" we might want to take here?

[Peter Clifton (pcjc2)]

Implementation comments...

I'd prefer to avoid using the vector_ APIs, as the code underneath is pretty dated (and by the feel of it, due being killed with fire). It is currently only used (as far as I can see) by the autorouter.

As we now depend on glib, and you can have GLists and other nice functionality from there at will. If I get time, I'll look at whether there is a case for migrating some of the users of the old vector_ code to something like that.

I've not reviewed the code in huge detail, but a cursory glance suggests that certain parts might be much simplified with a few of the nicer GLib string handling routines. (g_strdup_printf() is a lovely function to avoid lot of strlen calls and memory allocation grief).

Regards,

Peter Clifton

[Peter TB Brett (peter-b)]

I'm not sure what "similar sounding issue" you're referring to, exactly?

In my opinion, the obvious thing is for pcb to look at the files and just not display files that aren't footprints. Advantages:

- Does the right thing by default;
- Doesn't require users to figure out and manually hack yet another obscurely named file;
- Doesn't require users to manually keep the list of non-footprint files up-to-date when they add other files to the directory.

[KaiMartin (kmk-familieknaak)]

+1 to an implementation that shows "only footprints" as opposed to "everything except for...".

I'd prefer a solution that looksinto the file rather than at an expansion. This is more robust (and more unix style). Seems to me, the detection magic needs to look at the first file that does not start with "#". If this line starts with "Element", it most likely is a footprint.

---<)kaimartin(>---

[Peter Clifton (pcjc2)]

The concern I had with "automagically detecting" which files are footprints, is that for a big library, we would have to open every single file.

I've not benchmarked this, but my gut feel says this will not be fast - and we will end up wanting to cache the results, possibly persistently over runs of PCB. Not trivial to code this up.

[Peter TB Brett (peter-b)]

Well, there are two solutions. The simple/easy way is to only list files that have a ".fp" suffix, which would probably fix 90% of this bug.

The fashionable/better way is to do the scanning in a separate thread and populate the dialog as the results become available. A user would open the library window and would be able to interact with it, with library objects gradually appearing as the worker thread finishes scanning it.

[Bert Timmerman (bert-timmerman)]

Hi all,

Yeah, the easy way out of this bug is to use the extension filter ".fp"
and ask the users to consistently use this extension on all their footprint
files.

OTOH, one could follow "the road of most resistance" and (re)use code from:

http://cm.bell-labs.com/cm/cs/tpop/grep.c

and make it "--silent" and just return true or false on any given file in a directory.

Just the first hit on Google.

Maybe there is better code out on the net, there is lots of applications doing this thing.

Just my EUR 0.02.

Kind regards,

Bert Timmerman.

[Jeff Mallatt (jjm-j)]

Like Peter Clifton, I worried that examining the contents of all
candidate files would be slow. So I did just implement a quick
test of that. It opened every file, and searched for an Element
keyword.

On my machine, a library of 11000 files took 114 seconds to scan
when run immediately after a reboot. The second run took under
one second, because of OS caching no doubt.

Maybe that is an atypically large library, nevertheless the direct
approach appears to not be feasible.

[KaiMartin (kmk-familieknaak)]

The test only needs to be performed on files that are in the same directory. A lib that has 1e4 files in the same folder would not work well in the file chooser anyway. It would not work well in any chooser for that matter. Even full fledged file mamngers have a hard time to deal with such massively imbalanced filesystems. On my quad core desktop nautilus takes a nap of about 3 seconds when pointed to /usr/bin which is populated by a mere 2300 executables.

---<)kaimartin(>---

[Jeff Mallatt (jjm-j)]

All the newlib directories, wherever they are, are enumerated
and loaded into the Library window at application start-up time.