So… On Wed, Feb 13, 2019 at 11:31:53AM +0000, Dmitry Bogatov wrote: > [2019-02-02 17:27] Mattia Rizzolo <mat...@debian.org> > > Also, I'd like for you to check with the mentors.d.n code and make sure > > the template matches, keeping in mind that there is some kind of plan to > > make mentors send such mail automatically. > > While formatting was inspired by mentors.d.o, the whole idea of > debrequest is that it produce much more detailed template. Probably, > the most important feature is automatic deduction of programming > language. Also, dgit tries to accomodate to different workflows, like > dgit.
I don't think anything but the dgit bit would be problematic to see also
in mentors (where it wouldn't be relevant anyway, due to the nature of
dgit). Hence, I don't understand your issue with this.
I talked with the main person that is working on mentors' code with
(where I only merge, basically), and these are his comments:
- Split the code to a library so it can be imported easly by the tool
and mentors (import devscripts.debrequest)
- Make the Programming language detection/customisation in the
template, optional
I can imagine debexpo importing some python module from the locally
installed devscripts, so it would definitely be doable.
> Definitely, some things are quite opinionated (assumtion of dep-5, for
> example). Here is prelimitary patch. It builds, installs manpage and
> works for me. Could you please review it?
The requirement of dep-5 needs to be lifted, in the simple way that if
a dep-5 copyright is not avilable just the informations that come from
there won't be available.
> --- /dev/null
> +++ b/scripts/debrequest
> @@ -0,0 +1,112 @@
…
> + return list(Deb822.iter_paragraphs(open(path)))
…
> + changelog = Changelog(open('debian/changelog'))
[01:52:48 PM] <mapreri> + changelog = Changelog(open('debian/changelog'))
[01:52:48 PM] <mapreri> this doesn't leak fds, right?
[02:58:26 PM] <adsb> dunno, but I'd have put that in a context handler so I
didn't have to check
There you go. Even if it's fine, it's the kind of things that one
forgets if it doesn't write enough Python every day, so please make it
clearer.
> + try:
> + info['short_description'] = source_p['X-Short-Desc']
> + info['long_description'] = source_p['X-Long-Desc']
What are these fields?
> + build_depends = [dep.strip().split()[0]
> + for dep in source_p['Build-Depends'].split(",")
> + if dep != '']
How does this behave if somebody doesn't have a Build-Depends field?
And what about Build-Depends-Indep and Build-Depends-Arch?
> + if isfile('configure.ac') or isfile('configure') or glob('*.c') != []:
> + info['languages'].append('C')
autoconf can be used for a wider range of languages, also people could
rise eyebrows at putting toghether C and C++.
Also, I think the "configure" file by itself doesn't imply anything.
> --- /dev/null
> +++ b/scripts/debrequest.1
I think the whole documentation (and the template) require a fair bit of
en_* fixups, i.e. make it more grammatically correct.
> +debrequest \- generate Debian RFS and ITP requests mails
I just received the other day two bugs asking to expand such acronyms,
see #920869 and #920870. Please comply here as well.
> +.SH DESCRIPTION
> +.B debrequest
> +command line program collects machine-readable information
Either "The debrequest command line program collects" or "debrequest
collects" (I prefer the second).
> +from files in debian/ directory of source package located in
...in *the* debian/ directory...
> +in current directory and use is to generate RFS or ITP request.
...directory of the current source package...
However I would probably reflow the whole sentence, following what is
done in —for example— nmudiff(1):
debrequest should be run in the source tree of the pakcage being
worked on, to collect machine-readable information from the debian
packaging files and generate RFS or ITP requests.
(suggestion)
> +There are at least following assumptions about command line arguments
> +and source package:
> +.IP * 2
> +.IR debian/control
> +exists and is actually Debian source package control file
> +.IP "*" 2
> +.I .debian/copyright
> +follows
> +.B DEP-5
> +format and have following optional present:
> +.IR Upstream-Contact ,
> +.IR Upstream-Name .
> +.IP * 2
Really, drop these assumptions and make the program deal with them.
Either fail, or figure out what else to do.
> +Environment variables
> +.I DEBEMAIL
> +and
> +.I DEBFULLNAME
> +are set to reasonable values.
Where are this used? maintainer_namea and maintainer_email come from
the changelog, don't they?
> +Probably, there are more. If any of these assumptions are violated,
> +you will receive Python error message, like this:
Erm, please drop this.
If there are more, it's a bug that should be reported and fixed, not
telling the user that the program is most likely buggy...
> +Patches to improve error reporting are welcome.
…like I think for everything but the most weird software that explicitly
say they don't like patches…
> +.PP
> +Mandatory positional argument, which either
> +.B rfs
> +either
> +.B itp
> +specifies what kind of request to generate.
Here I can't read this sentence.
I recomment to move this paragraph at the top of the manpage, as that's
where you usually describe the command line options.
Also, the syntax construction is "either <option1> or <option2>", not
"either … either …". So:
The mandatory positional argument is either rfs or itp, to specify
what kind of request to generate.
> +.SH EXAMPLES
> +Here is example of RFS for
> +.I debrequest
> +itself:
I'm not sure I'd include this in the manapage: it's bound to become
outdated as soon as anybody changes a bit the template…
> --- /dev/null
> +++ b/scripts/devscripts/templates/debrequest-itp.j2
> +{# -*- jinja2 -*- #}
> +From: "{{maintainer_name}}" <{{maintainer_email}}>
> +To: Debian Bug Tracking System <sub...@bugs.debian.org>
> +Subject: ITP: {{ upstream_name }} -- {{ short_description }}
> +X-Debug-Cc: debian-de...@lists.debian.org
As I also mentioned in my previous email (did you miss it?), this is the
wrong header. It's called X-Debbugs-Cc.
> +{% if 'Haskell' in languages %}
> + I plan to maintain this package as part of Debian Haskell Group
> +{% else %}
> + {% if 'Emacs Lisp' in languages %}
> + I plan to maintain this package as part of Emacsen team
> + {% else %}
> + I plan to maintain this package myself, keeping debianization in following
> + Git repository:
> +
> + {{ vcs_git }}
> + {% endif -%}
> +{% endif -%}
Mhh.
Why do you thin all haskell packages ever are in the team, and ditto for
lisp ones?
Sure, in those two cases most of the packages written in that language
are in there, but I wouldn't want to push it this forcefully.
For this, I'd suggest a flag where you can specify a team.
And I'd always write out the git repository, no matter if it's in a team
or not.
> --- /dev/null
> +++ b/scripts/devscripts/templates/debrequest-rfs.j2
> @@ -0,0 +1,68 @@
> +{# -*- jinja2 -*- #}
> +From: {{ maintainer_name }} <{{ maintainer_email }}>
> +To: sub...@bugs.debian.org
> +{% if upload_count == 1 -%}
> +{% set tag = 'ITP' -%}
> +{% else -%}
> +{% set tag = '' -%}
> +{% endif -%}
> +Subject: RFS: {{ package_name }}/{{ version }} {{ tag }}
this would add a trailing whitespace in case of no tag.
also, the ITP "tag" should be "[ITP]", in square brackets.
> +Package: sponsorship-requests
> +Severity: wishlist
wishlist is only for ITP packages, otherwise is normal (i.e. don't set
the severity, leave the default).
> +It builds those binary packages:
s/those/these/, but ISTR jinja2 has something to deal with
pluralization, in which case it's either "this" for one package, or
"these".
> +This package succesfully builds on debomatic machine:
> +
> + http://debomatic-i386.debian.net/distribution#unstable/{{ package_name
> }}/{{ version }}
Next to nobody has access to debomatic, compared with the amount of
users that would end up using this tool, so this is hardly appropriate.
Also, ISTR debomatic drops results after 1 month (and it's easy to
overwrite/delete them anyway), so it's also not good for RFS bugs for
ITPs, that easy stall for months.
> +{% if dgit %}
> +Please note, that package is maintained with dgit(1) tool
> +using dgit-maint-merge(7) workflow. For more information about how to
> +sponsor this package, see dgit-sponsorship(7).
> +
> +With /bin/sh following commands should suffice:
Please drop the shell suggestion. A sponsor runs commands the way he
likes more.
> + $ git clone {{ vcs_git }} {{ package_name }}
Actually, no. Where did you figure this out?
The workflow is slightly different between ITPs and non-ITPs, and for
the letter the starting point is using `dgit clone`, followed by either
an import of the dsc, or adding a git remote with the remote indicated
by the sponsee.
> + $ cd {{ package_name }}
> + $ make -f debian/rules get-orig-source # 'gbp buildpackage' is fine
You are aware that the get-orig-source target:
1) has just been removed from policy
2) lintian started to push for removal of trivial ones
3) next to nobody implement it anyway
so what kind of suggestion is that? :)
In that note, I wish to advertise `mkorigtargz`, which needs to learn
about `gbp export-orig` (why would you write `gbp buildpackage` over
there?), but otherwise tends to behave quite well.
> + $ dgit sbuild
Or build-source. Or whatever.
Really, I suggest you drop all this dgit stuff.
People using dgit will already know what to do, and those that don't
should really reaed the manpage in full before ever touching dgit.
> +Alternatively, one can download the package with dget using this command:
> +{% set letter = package_name[0] %}
> + dget -x https://mentors.debian.net/debian/pool/main/{{ letter }}/{{
> package_name }}/{{ package_name }}_{{ version }}.dsc
this breaks:
* for libraries, which don't use only the first letter 'l'
* packages using epochs
> +{% if vcs_git is defined %}
> +Alternatively, you can access package debian/ directory via git from URL:
> + {{ vcs_git }}
> +{% endif %}
Honestly, as a sponsor I tend to expect the git repository to contain
the full sources unless told otherwise, and afaik that's the general
norm except for the haskell team, so please drop that note about the
debian/ directory.
> --- a/scripts/setup.py
> +++ b/scripts/setup.py
> @@ -24,6 +24,7 @@ if __name__ == '__main__':
> name='devscripts',
> version=get_version(),
> scripts=SCRIPTS,
> - packages=['devscripts', 'devscripts/test'],
> + packages=['devscripts', 'devscripts/test', 'devscripts/templates'],
> + package_data={'devscripts': ['templates/*']},
There already is a templates directory in the root. Do you think you
could work on merging the two? (Which could mean to move the other one,
sure).
Also, this requires adding an entry in README, as well as double
checking (build-)depends and/or recommends and/or suggests (like,
nothing will pull jinja2, I think).
--
regards,
Mattia Rizzolo
GPG Key: 66AE 2B4A FCCF 3F52 DA18 4D18 4B04 3FCD B944 4540 .''`.
more about me: https://mapreri.org : :' :
Launchpad user: https://launchpad.net/~mapreri `. `'`
Debian QA page: https://qa.debian.org/developer.php?login=mattia `-
signature.asc
Description: PGP signature
