Discussion:
[janitorial] Text file style convention "enforcement"
(too old to reply)
Olaf Meeuwissen
2017-05-31 12:41:40 UTC
Permalink
Raw Message
Hi all,

It's been a while since my last janitorial. Interest in contributing to
the project comes in waves. I've been preoccupied with the preparations
for the 1.0.27 release. During which, Aaron made a suggestion about how
coding style conventions could help spot bugs[1]. I followed up with an
"apologetic" reply[2] why I didn't want to enforce a *coding* style, but
at the same time mentioned some, hopefully, non-controversial rules for
all text files.

[1] https://lists.alioth.debian.org/pipermail/sane-devel/2017-May/035353.html
[2] https://lists.alioth.debian.org/pipermail/sane-devel/2017-May/035355.html

Well, I applied those rules and pushed the result. Here are the rules:

- all text files shall be UTF-8 encoded
- all lines shall end in non-whitespace
- all files shall end with a newline
- all files shall end with a non-empty line

# I wondered what to do with leading empty lines but decided not to
# touch these (for now). They might serve some sort of "aesthetic"
# purpose. I also noticed a note on "follow the GNU coding standards"
# in doc/backend-writing.txt (and shied away from that, for now?). As
# for the tab vs spaces mess? Let's leave that for later.

Cool. Now what? How does this affect you? How do I "oblige" to this?
How do you "enforce" this?

First, the effect on people working with the SANE backend repository.
Any of your changes may now cause conflict when you try to merge with,
rebase on or apply patches to the `master` branch. Most of the pain can
be alleviated with the various `--ignore-*` flags to the command you use
to do that. The `git help` comman for your action of choice has details
on how this works, as does the `patch` manual page.

# If you use a GUI to do this, check its documentation. There should be
# an option to set those flags, on a one-time basis or as a preference.
# If not, submit a feature request for that GUI.

The "rules" are laid down in a `.editorconfig` file in the repository.
If your editor or IDE is supported by EditorConfig, you're all set. If
your editor or IDE needs a plugin, do yourself a favour and install it.
The EditorConfig[3] documentation has info on supported plugins[4].
It's not difficult and works wonders. Been there, done that myself and
am now happily using the Emacs plugin and Emacs now makes sure I follow
the "rules".

[3] http://editorconfig.org/
[4] http://editorconfig.org/#download

If your editor or IDE is not supported by EditorConfig, do not despair.
First submit an issue[5] asking for a "new plugin" (use the label!) ;-)

[5] https://github.com/editorconfig/editorconfig/issues/new

While waiting for that plugin (or hacking up one yourself), you can use
the `tools/style-check.sh` script to check files. It also has a `--fix`
option that will take care of the newline and whitespace issues. Seeing
the encoding cannot be guessing automagically, any compliance violations
for the UTF-8 rule need to be handled manually.

Want to make this even simpler? No sweat. Just add the attachment to
your `.git/hooks/pre-commit` file (or add it if you don't have one).
Note that the file needs execute permissions. A simple

chmod 0755 .git/hooks/pre-commit

will do that trick.

# For those of you that don't have an `xargs` that supports the `-r`
# option, which is a GNU extension, you can rewrite the pipe to use a
# `while read file` loop. Don't know how? Just ask!
# Parallelization afficionados can play with the `-P` and `-n` options
# to `xargs` if so inclined.

The pipeline over at GitLab.com[6] will shortly (tomorrow?) be modified
to run the `tools/style-check.sh` before running any builds. Failing
that check will torpedo the build, just like compiler warnings on Debian
stable.

[6] https://gitlab.com/sane-project/backends/pipelines

Hope this helps (and doesn't ruffle too many feathers ;-)
--
Olaf Meeuwissen, LPIC-2 FSF Associate Member since 2004-01-27
GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13 F43E B8A4 A88A F84A 2DD9
Support Free Software https://my.fsf.org/donate
Join the Free Software Foundation https://my.fsf.org/join
Aaron Muir Hamilton
2017-05-31 21:14:30 UTC
Permalink
Raw Message
Post by Olaf Meeuwissen
Hi all,
It's been a while since my last janitorial. Interest in contributing to
the project comes in waves. I've been preoccupied with the preparations
for the 1.0.27 release. During which, Aaron made a suggestion about how
coding style conventions could help spot bugs[1]. I followed up with an
"apologetic" reply[2] why I didn't want to enforce a *coding* style, but
at the same time mentioned some, hopefully, non-controversial rules for
all text files.
[1] https://lists.alioth.debian.org/pipermail/sane-devel/2017-May/035353.html
[2] https://lists.alioth.debian.org/pipermail/sane-devel/2017-May/035355.html
- all text files shall be UTF-8 encoded
- all lines shall end in non-whitespace
- all files shall end with a newline
- all files shall end with a non-empty line
# I wondered what to do with leading empty lines but decided not to
# touch these (for now). They might serve some sort of "aesthetic"
# purpose. I also noticed a note on "follow the GNU coding standards"
# in doc/backend-writing.txt (and shied away from that, for now?). As
# for the tab vs spaces mess? Let's leave that for later.
Cool. Now what? How does this affect you? How do I "oblige" to this?
How do you "enforce" this?
First, the effect on people working with the SANE backend repository.
Any of your changes may now cause conflict when you try to merge with,
rebase on or apply patches to the `master` branch. Most of the pain can
be alleviated with the various `--ignore-*` flags to the command you use
to do that. The `git help` comman for your action of choice has details
on how this works, as does the `patch` manual page.
# If you use a GUI to do this, check its documentation. There should be
# an option to set those flags, on a one-time basis or as a preference.
# If not, submit a feature request for that GUI.
The "rules" are laid down in a `.editorconfig` file in the repository.
If your editor or IDE is supported by EditorConfig, you're all set. If
your editor or IDE needs a plugin, do yourself a favour and install it.
The EditorConfig[3] documentation has info on supported plugins[4].
It's not difficult and works wonders. Been there, done that myself and
am now happily using the Emacs plugin and Emacs now makes sure I follow
the "rules".
[3] http://editorconfig.org/
[4] http://editorconfig.org/#download
If your editor or IDE is not supported by EditorConfig, do not despair.
First submit an issue[5] asking for a "new plugin" (use the label!) ;-)
[5] https://github.com/editorconfig/editorconfig/issues/new
While waiting for that plugin (or hacking up one yourself), you can use
the `tools/style-check.sh` script to check files. It also has a `--fix`
option that will take care of the newline and whitespace issues. Seeing
the encoding cannot be guessing automagically, any compliance violations
for the UTF-8 rule need to be handled manually.
There's also GNU indent, which defaults to GNU style these days, and
should be installed on most of our workstations (except perhaps on
OpenBSD or FreeBSD, but possibly there as well).

It might be interesting to ask people to run it on a file when they make
a considerable change and there are no known outstanding patches against
the file. Or even if there are outstanding patches which conflict with
style repairs, one can format the patch branch and it should apply.

Honestly my greatest concern is that the genesys_gl*.c per-controller
backends will drift further apart. The 800-series ones are still very
similar, but the 124[+] backend has some of the function definitions
shifted around in order, but otherwise nearly identical in function.
I'm trying to get my hands on a scanner matching each genesys controller
revision so that I can regression test anything I do in that realm.

Those files are largely identical, the functional differences are
limited to a few differing or new function definitions, and the magic
values used in given registers (though the register offsets remain
largely the same).

The more the files drift apart, the harder it'll be to a) realize that
fixes should be ported between them and b) that they are substantially
similar, neigh on identical files.

Anyway, if all goes well I should be receiving four more genesys-based
scanners, including a couple not mentioned on the website. I'll also be
getting a whole stack of calibration targets so I can tell if something
has gone subtly wrong for a given mode on a given backend. :- )

Boy, that was a bit off-topic, do go on.
Post by Olaf Meeuwissen
Want to make this even simpler? No sweat. Just add the attachment to
your `.git/hooks/pre-commit` file (or add it if you don't have one).
Note that the file needs execute permissions. A simple
chmod 0755 .git/hooks/pre-commit
will do that trick.
# For those of you that don't have an `xargs` that supports the `-r`
# option, which is a GNU extension, you can rewrite the pipe to use a
# `while read file` loop. Don't know how? Just ask!
# Parallelization afficionados can play with the `-P` and `-n` options
# to `xargs` if so inclined.
The pipeline over at GitLab.com[6] will shortly (tomorrow?) be modified
to run the `tools/style-check.sh` before running any builds. Failing
that check will torpedo the build, just like compiler warnings on Debian
stable.
[6] https://gitlab.com/sane-project/backends/pipelines
Hope this helps (and doesn't ruffle too many feathers ;-)
--
Olaf Meeuwissen, LPIC-2 FSF Associate Member since 2004-01-27
GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13 F43E B8A4 A88A F84A 2DD9
Support Free Software https://my.fsf.org/donate
Join the Free Software Foundation https://my.fsf.org/join
--
sane-devel mailing list: sane-***@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
to sane-devel-***@lists.alioth.debian.org
Olaf Meeuwissen
2017-06-01 13:14:57 UTC
Permalink
Raw Message
Hi Aaron,

Thanks for the feedback. Details inlined below.
Post by Aaron Muir Hamilton
Post by Olaf Meeuwissen
[...]
# I wondered what to do with leading empty lines but decided not to
# touch these (for now). They might serve some sort of "aesthetic"
# purpose. I also noticed a note on "follow the GNU coding standards"
# in doc/backend-writing.txt (and shied away from that, for now?). As
# for the tab vs spaces mess? Let's leave that for later.
There's also GNU indent, which defaults to GNU style these days, and
should be installed on most of our workstations (except perhaps on
OpenBSD or FreeBSD, but possibly there as well).
It's not the tool that's the problem. There's lots of code "beautifier"
programs to choose from (too many maybe). The problem is that backend
maintainers have their own preferences and they may not be all the same.

I can ride roughshod over everybody, convert all code files to the GNU
coding standards in a jiffy with automated checking to boot and whack
complainers over the head with a quote from doc/backend-writing.txt but
that's not gonna make me (m)any friends ;-)
Post by Aaron Muir Hamilton
It might be interesting to ask people to run it on a file when they make
a considerable change and there are no known outstanding patches against
the file. Or even if there are outstanding patches which conflict with
style repairs, one can format the patch branch and it should apply.
Backend maintainers are free to reformat/sanitize the code for *their*
backend whenever they like, IMHO. When co-maintaining, please get some
kind of agreement between the co-maintainers first. Whatever style you
choose, pick something that's easily checked(/fixed) by a common tool.

# People may want to add something to their pre-commit hook. For the
# builder(s), if it's in Debian stable that's good enough.
Post by Aaron Muir Hamilton
Honestly my greatest concern is that the genesys_gl*.c per-controller
backends will drift further apart. The 800-series ones are still very
similar, but the 124[+] backend has some of the function definitions
shifted around in order, but otherwise nearly identical in function.
I'm trying to get my hands on a scanner matching each genesys controller
revision so that I can regression test anything I do in that realm.
Those files are largely identical, the functional differences are
limited to a few differing or new function definitions, and the magic
values used in given registers (though the register offsets remain
largely the same).
Sounds like a case of source code cloning. That doesn't scale. Been
there, done that. It's hell. Refactor the common functions into a
single file, parameterize the differences and you should be able to get
rid off a fair bit of code.

I haven't looked at the code much but did notice similarities while
fixing all the compiler warnings. Maybe we should run a copy-n-paste
detector over the code base.
Post by Aaron Muir Hamilton
The more the files drift apart, the harder it'll be to a) realize that
fixes should be ported between them and b) that they are substantially
similar, neigh on identical files.
Don't port fixes between them. Use a single copy of the code for all of
them. Just put each duplicated function in its own separate .c file and
include that directly (or declare the function extern), whatever works.

You can do this in a separate branch so others can give feedback. I'll
be happy to take a peek and give feedback on the general direction. I
won't be able to check all the little details.
Post by Aaron Muir Hamilton
Anyway, if all goes well I should be receiving four more genesys-based
scanners, including a couple not mentioned on the website. I'll also be
getting a whole stack of calibration targets so I can tell if something
has gone subtly wrong for a given mode on a given backend. :- )
But if you have lots of hardware to test with ... those details should
be taken care of ;-)
Post by Aaron Muir Hamilton
Boy, that was a bit off-topic, do go on.
By all means, do get off-topic, or rather off-on-a-tangent, every once
in a while. The list could do with some more developer talk ;-)

Hope this helps,
--
Olaf Meeuwissen, LPIC-2 FSF Associate Member since 2004-01-27
GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13 F43E B8A4 A88A F84A 2DD9
Support Free Software https://my.fsf.org/donate
Join the Free Software Foundation https://my.fsf.org/join
--
sane-devel mailing list: sane-***@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
to sane-devel-***@lists.alioth.debian.org
Loading...