1. proposed patch adding the infrastructure
   0001-autotools-initial-autotools-plugin-commit.patch

Trying to review the review tool :)

Does tests run? Yes, with patch after setting group to 'Generic.autotools'.

Basic text, url, type sane? No.

• Since there is no GL reference the type of this test is EXTRA, not SHOULD.
• An url would be good.
• Text need to be rephrased to something which can be OK/Fail e. g., "Package should not use obsolete m4 macros"

Is there a test case? No.
We need to add one or two. First question: what is the expected outcome of this test run by test_check today (besides the test_scriptlet* tests)?
The largest block of untested lines is 254-283.

pep8? OK

pylint? OK

Nit-picks: The group must not be 'Generic' but rather 'Generic.autotools' as suggested in the template. Needs an update in the test file, will push a git branch for this. (this requirement might be considered a bug BTW).

Commenting style: More comments does not hurt, but examples like
{{{
# construct error database
self.err_db = self.construct_err_db()
}}}
are not helpful, just confusing. Please read the pep8 GL http://www.python.org/dev/peps/pep-0008, note the chapters on
- commenting
- blank lines
- the ordering of constants, functions and classes in a module.

The docstring "''' wrap the text on the 76th character '''" is wrong.

Did I say this seems like a nice addition?

EDIT 1: The line 'self.set_passed(self.FAIL, msg, [output])' is wrong, the last argument is a list of Attachment, not a list of strings.

EDIT2: Reviewing the generated attachment I think it's a little to verbose. If the general text about using -Wobsolete, the replacement etc. was available through the test's url this text could be trimmed down considerably, basically just saying "Obsolete macro XX in file YY. Perhaps it would make sense to create a specific page e. g., on the f-r wiki and use that as url? Thinking about it, I'm not sure an attachment is needed at all if walking this path.

Replying to [comment:1 leamas]:

Trying to review the review tool :)

Thanks! I will fix all you mentioned here soon. Just couple of questions:

• An url would be good.

Yes - it would. Where exactly could I put some info about this new checker, are there examples on fedora-review wiki? I guess it should be somewhere here.. as the EXTRA is really proper category.

Is there a test case? No.

Will look at this. Currently, 'gc' package exercises this checker pretty good.

EDIT2: Reviewing the generated attachment I think it's a little to verbose. If the general text about using -Wobsolete, the replacement etc. was available through the test's url this text could be trimmed down considerably, basically just saying "Obsolete macro XX in file YY. Perhaps it would make sense to create a specific page e. g., on the f-r wiki and use that as url? Thinking about it, I'm not sure an attachment is needed at all if walking this path.

This is truth - it is quite verbose. Another possibility would be to create some "autotools-check.log" file to clean the review file, or?

Thanks,
Pavel

Replying to [comment:2 praiskup]:
[cut]

This is truth - it is quite verbose. Another possibility would be to create some "autotools-check.log" file to clean the review file, or?

There are actually two issues here. The first is the size if the report, I think we agree on this.

The second is what f-r does, and doesn't. To put it simple, f-r just reports problems, never suggests what to do with them. Given this, I think this should be presented with some plain text without attachments like "Obsolete macros: AC_BAD_ONE in configure.in. AM_BAD_ONE1, AM_BAD_ONE2 in Makefile.am" Although this list theoretically could be long, I doubt this is a practical concern(?).

Next part would then be a proper URL with the useful info you now have in the attachment. A simple solution would be to have this in the f-r wiki, please feel free to add a page if you like. I don't think adding this to a separate file like autotools-check.log is the right way for reasons above.

The basic problem here is that there are no official autotools GL from FPC. There seem to have been a lot of discussions about this which ended up in nothing. This is a shame IMHO.

EDIT: Since there probably won't be any attachment, skip the test discussion for now.

Ping?!

Iteration #2
0001-autotools-initial-autotools-plugin-commit.2.patch

Hi, sorry for so long delay. I tried to fix everything you noticed. Except the test case (as you stopped the discussion :)).

There is a problem with 'url'. How can I force f-r to show the url when something is found?

Thanks for your future comments, Pavel

Replying to [comment:5 praiskup]:

Hi, sorry for so long delay.
NP :)

I tried to fix everything you noticed. Except the test case (as you stopped the discussion :)).

I've lost track a little here... but will push a branch w testcases

There is a problem with 'url'. How can I force f-r to show the url when something is found?

The url is displayed as soon as you return self.FAIL

Thanks for your future comments, Pavel

I will take a better look at this, hopefully with a somewhat smaller delay ;)

BTW: Nice webpage! That said, sorry for my bad French: autoconfiscated?!

I have pushed a feature branch issue-206 based on your patch. It's a minor fix (file rename) + some checks.

Actually, this revealed a bug in current code: the url was not displayed even though the test failed if the test was not an issue. I have pushed a fix for this on devel. Thanks for heads-up!

Basically I think this is going well, we are close to actually commit this. Some fixes needed:
- Mark module level constants and functions as private. OBS_M4S_AUTOMAKE -> _OBS_M4S_AUTOMAKE,
prepend_indent -> _prepend_indent etc.
- Note the ordering of constants, methods and classes in pep-0008, move global methods before
all classes.
- Likewise, move all nested methods to the top of the enclosing method. E. g., def shorter_configure
should be the first statement in trace(self)
- The unit test fails. I think you are in a better position than me to iron out the details.
- Please consider adding another unittest (another specfile) modeled from the first.
- Fix the misspelled check-name CheckAutotoolsObsoleteMarcos (also in tests...)
- Please consider using another word than trace e. g., in self.trace(). Trace is conventionally used
for debug outputs and similar things rather than digging out tokens from a text.
'Parse' is probably a better choice here.

I have pushed a feature branch issue-206 based on your patch. It's a minor
fix (file rename) + some checks.

Thanks :) there was the branch 'autotools' already, but never mind. I'm
working with issue-206 now. I need a little help regarding the tests, once
it gets fixed, I'll re-send patch.

Actually, this revealed a bug in current code: the url was not displayed
even though the test failed if the test was not an issue. I have pushed a
fix for this on devel. Thanks for heads-up!

Thanks for the fix! :)

Basically I think this is going well, we are close to actually commit
this. Some fixes needed:
- Mark module level constants and functions as private. OBS_M4S_AUTOMAKE
-> _OBS_M4S_AUTOMAKE,
prepend_indent -> _prepend_indent etc.
- Note the ordering of constants, methods and classes in pep-0008, move
global methods before
all classes.

Done.

• Likewise, move all nested methods to the top of the enclosing method. E.
  g., def shorter_configure
  should be the first statement in trace(self)

Done.

• The unit test fails. I think you are in a better position than me to
  iron out the details.

here I need your help. Where can I find the variable having the
absolute path of source files? When I'm pointing user to some
configure.ac file, I would like do it relatively to source root. The
src.containers[0] in trace() method works for production environment, but
fail in test environment.. Could you point me somewhere?

• Please consider adding another unittest (another specfile) modeled from
  the first.

Will try to prepare some arbitrary configure.ac file.

• Fix the misspelled check-name CheckAutotoolsObsoleteMarcos (also in
  tests...)

Wow, fixed. It was hard to me to find where you see the problem even when you
pointed me. Switched to CheckAutotoolsObsoletedMacros
^ ^^

• Please consider using another word than trace e. g., in self.trace().
  Trace is conventionally used
  for debug outputs and similar things rather than digging out tokens from
  a text.
  'Parse' is probably a better choice here.

If you insist on that, I will pick "parse" - but in auto* dialect, the
proper word is trace, also see the 'autom4te --trace'.

Pavel

First of all: I havn't noticed your reply until now (something is broken, I get no email). Please notify me as soon as you need help (leamas dot alec at gmail com or leamas on #fedora-review). Sorry for the delay.

The paths... have you looked into ReviewDirs? ReviewDirs.root is the working dir, ReviewDirs.root/BUILD is the symlink to the sources (which lives in a single directory below BUILD).

Is this what you are looking for?

I will not insist on using 'Parse'. Seems that you have considered a name and come to a conclusion. So let's drop this.

Replying to [comment:9 leamas]:

First of all: I havn't noticed your reply until now (something is broken, I get no email). Please notify me as soon as you need help (leamas dot alec at gmail com or leamas on #fedora-review). Sorry for the delay.

No problem! :)

The paths... have you looked into ReviewDirs? ReviewDirs.root is the working dir, ReviewDirs.root/BUILD is the symlink to the sources (which lives in a single directory below BUILD).

Is this what you are looking for?

Absolutely. Thanks. I'll attach new patch in a moment.

Patch against 'issue-206' branch.
0001-autotools-fix-Leamas-s-review-notes.patch

Once again I lost this, no emails sent.

All looks good. merging and pushing. Thanks for fixing this

Fixed in 8c74a3a.