[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Review of debputy editor provided docs for packagers



Richard Lewis wrote:

Thanks for the extra pair of eyes!

Hi

Thanks indeed.

I forgot to mention that I would like to be CC'ed as that makes it easier for me.

#. completion (etc.)
#: src/debputy/lsp/data/debian_control_reference_data.yaml
msgctxt "Stanza:Package|Field:Description"
msgid "Package synopsis and description"
msgstr ""

#. [Hover Doc] Extended description for the field itself [Markdown]. Shown in
#. hover docs (etc.)
#: src/debputy/lsp/data/debian_control_reference_data.yaml
#, python-brace-format
msgctxt "Stanza:Package|Field:Description"
msgid ""
"A human-readable description of the package. This field consists of two related but distinct parts.\n"

"related but distinct" seems unneccssary here

Oh, yes, I commented about the "several distinct but very important
rules" later but this line didn't catch my attention, possibly because
the relationship between the two parts of a package description really
is a bit subtle and might deserve some verbiage.


For now, I have reduced it to "This field consists of two parts."

[...]
You should add a link to Justin's style guide to help people write these better!

And I should give that style guide a proper overhaul.

I am happy to do that. Which link should I use?

#. [Synopsis] One-line description for the value "yes" [Plaintext]. Shown with
#. completion (etc.)
#: src/debputy/lsp/data/debian_control_reference_data.yaml
msgctxt "Stanza:Package|Field:Protected"
msgid "Prevent trivial uninstallation of the package"
msgstr ""

why "trivial", is there a "non-trivial" installation ??

As in, uninstalling the package just on a whim expecting the results
to be trivial.  Maybe the word ought to be "casual"?


Amended to use "casual".

#. [Hover Doc] Extended description for the field itself [Markdown]. Shown in
#. hover docs (etc.)
#: src/debputy/lsp/data/debian_control_reference_data.yaml
#, python-brace-format
msgctxt "Stanza:Package|Field:Homepage"
msgid ""
"Link to the upstream homepage for this binary package.\n"

in the other message it used URL (mostly)

And since the field is called "homepage", the word homepage is a bit
redundant.  The synopsis says "Upstream homepage URL for this binary
package" while the hover-docs have "Link to the upstream homepage for
this binary package"... Why not the other way round, a nice concise
"Upstream URL for this binary package" in the synopsis then "Link" (or
rather: it "Links") "to the upstream homepage URL for this binary
package" in the hover-docs.

Thanks, amended.

#. [Synopsis] One-line description of the field itself [Plaintext]. Shown with
#. completion (etc.)
#: src/debputy/lsp/data/debian_copyright_reference_data.yaml
msgctxt "Stanza:Header|Field:Format"
msgid "Declare the file uses the DEP-5 specification and which version"
msgstr ""

  msgid "Declares that the file uses the DEP-5 specification, and which version"

Seems like an incomplete sentence to me, which version of what - the package or DEP-5?

If it had more room it could be "Declares that the file uses the DEP-5
specification, and if so, which DEP-5 version". [...]


I have inserted the long version for now.

There is no hard limit in the protocol on the line length, so maybe it is a non-issue with the length. And if not, I will get a bug report and we can work from there.

#. [Hover Doc] Extended description for the field itself [Markdown]. Shown in
#. hover docs (etc.)
#: src/debputy/lsp/data/debian_copyright_reference_data.yaml
#, python-brace-format
msgctxt "Stanza:Header|Field:Format"
msgid ""
"URI of the format specification. The field that should be used for the current version of this\n"
"document is:\n"
"\n"
"**Example**:\n"
"```\n"
"Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/\n";;
"```\n"
"\n"
"The original version of this specification used the non-https version of this URL as its URI, namely:\n"
"\n"
"```\n"
"Format: http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/\n";;

Do we need to know the stuff about the old url?

Maybe: it's explaining that the outdated URL has been "grandfathered
in" as valid to avoid making old packages unnecessarily buggy.


That was exactly what I was going for. Explaining that people might see the http variant and it would still be valid.

At some point, `debputy` will be emitting diagnostics for this field. One of those could be replacing `http://` with `https://`. I could accelerate that and remove this mention of the `http://` variant if you think that would make the hover doc easier to understand.


#. [Synopsis] One-line description of the field itself [Plaintext]. Shown with
#. completion (etc.)
#: src/debputy/lsp/data/debian_copyright_reference_data.yaml
msgctxt "Stanza:Header|Field:License"
msgid "Provide license information for the package as a whole"

why "as a whole"? does some other field provide it for part of a
package? do you mean "source package"?

The system is, you can define an "overall" license, and then pick out
particular components as exceptions (since they might have different
contributors and copyright dates and so on).

Indeed. As far as I understand, it is for the niche case where the upstream have multiple license options and Debian, for some reason, want to state that "We picked license X out of all the options" (or Debian's package will de facto be using X out of the options). Most likely accompanied with a `Comment` field stating why that option was picked.

This can matter to derivatives or consumer in case they are subject to license restrictions. As an example, a private company might not want to link against software under the GPL license, because they do not want to be subject to the license terms but might be fine with LGPL.

Due to how LGPL works, you are allowed to "upgrade" (replace) LGPL into with) GPL and Debian could be relying on that in some cases where Debian compiles the package against an optional GPL-only library.

In this case, Debian could provide a DEP-5 file like:

```
Format: ...
License: GPL-2+
Comment: Some rationale for replacing LGPL with GPL.

Files: *
Copyright: ...
License: LGPL-2+

...
```

Though, I am not aware of many Debian people spending this level of effort on their `debian/copyright` file. So I am not sure how much it is used in practice.

  "Using `Copyright` in the `Header` stanza is useful when it records a notable difference or simplification\n"
  "of the other files' `Copyright` fields, but it is not useful when it merely aggregates those fields.\n"

"\n"
"Any formatting is permitted. Simple cases often end up effectively being one copyright holder per\n"
"line; see the examples below for some ideas for how to structure the field to make it easier to read.\n"
"\n"
"If a work has no copyright holder (i.e., it is in the public domain), that information should be recorded\n"

is "i.e" correct - arnt there other ways it could have no copyright holder, eg being old enough?

It is at least closer to "i.e." than "e.g." - if it's old enough for
copyright to have expired then it becomes public domain.  It's
possible for things to not be covered by copyright because they're too
trivial to count as creative works, but then again it's normal to call
those "public domain" too.


For what it is worth, that particular line was taken verbatim from https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/#copyright-field


#. [Hover Doc] Extended description for the field itself [Markdown]. Shown in
#. hover docs (etc.)
#: src/debputy/lsp/data/debian_copyright_reference_data.yaml
#, python-brace-format
msgctxt "Stanza:Header|Field:Files-Excluded"
msgid ""
"Remove the listed files from the tarball when repacking (commonly via `uscan`). This can be useful\n"
"when the listed files are non-free but not necessary for the Debian package. In this case, the\n"
"upstream version of the package should generally end with `~dfsg` or `+dfsg` (to mark the content\n"

can you pick one of ~dfsg or +dfsg to recommend?

They have different effects, so the circumstances where you'd use them
get several paragraphs of explanation in Debian Policy 5.6.12.2.

Indeed.

I suppose there would be no harm in having a paragraph explaining when to use which. Here is my draft for how to include it (full hover doc). It also includes an example with the component case as suggested by Justin below:

              Remove the listed files from the tarball when repacking (commonly via `uscan`). This can be useful
              when the listed files are non-free but not necessary for the Debian package. In this case, the
              upstream version of the package should generally end with `~dfsg` or `+dfsg` (to mark the content
              changed due to the Debian Free Software Guidelines). The exclusion can also be useful to remove
              large files or directories that are not used by Debian or pre-built binaries. In this case, `~ds`
              or `+ds` should be added to the version instead of `~dfsg` or `+dfsg` for "Debian Source" to mark
              it as altered by Debian. If both reasons are used, the `~dfsg` or `+dfsg` version is used as that
              is the more important reason for the repacking.

              **Example**:
              ```
              Files-Excluded: exclude-this
                exclude-dir
                */exclude-dir
                .*
                */js/jquery.js
              ```

              The `Files-Included` field can be used to "re-include" files matched by `Files-Excluded`.

              It is also possible to exclude files in specific "upstream components" for source packages
              with multiple upstream tarballs. This is done by adding a field called
              `Files-Excluded-<component>`. The `<component>` part should then match the component name
              exactly (case sensitive).
**Example**:
              ```
              # This field applies to the `foo` component tarball (`${SOURCE}_${VERSION}.orig-foo.tar.*`)
              # instead of the main orig tarball.
              Files-Excluded-foo: exclude-this
                exclude-dir
                */exclude-dir
                .*
                */js/jquery.js
              ```

              The field uses the same syntax as the `Files` fields from the `Files` stanzas.
Whether the package should use `~` (`~dfsg`) or `+` (`+dfsg`) in the version depends on
              the situation with the `+` variants being the most commonly used. If in doubt, look for
              an argument for why the `~` variants would be preferred in your case. In absence of any
              clear rationale for using `~` variants, then use the `+` variants. Review
              [section 5.6.12 of the Debian Policy] and its subsections for more information on the
              versioning and the special rules regarding the sort order for `~`.

              Defined by: `mk-origtargz` (usually used via `uscan`)

              [section 5.6.12 of the Debian Policy]: https://www.debian.org/doc/debian-policy/ch-controlfields.html#special-version-conventions
End draft.

I deliberately linked to 5.6.12 "and its subsections" rather than 5.6.12.2. My rationale was that 5.6.12 has the context about the sort order for `~` which is very different from standard (English) sort order. Therefore, I wanted to include the link to 5.6.12.

I also deliberately placed it late in the hover doc. The main topic here is `Files-Excluded`. There should be enough hints about the version to ensure people are aware of the interaction and preferably guide them to the right case.

I think it would be more ideal if there was a page I could link to that explained this case better. At that point, I could probably inline it in the first paragraph with a "Review X for details on when to use dfsg vs. ds and whether to use ~ or +". That page might exist, but I cannot find it if it does.


"**Example**:\n"
"```\n"
"Files-Excluded: exclude-this\n"
"  exclude-dir\n"
"  */exclude-dir\n"
"  .*\n"
"  */js/jquery.js\n"
"```\n"
"\n"
"The `Files-Included` field can be used to \"re-include\" files matched by `Files-Excluded`.\n"
"\n"
"It is also possible to exclude files in specific \"upstream components\" for source packages\n"
"with multiple upstream tarballs. This is done by adding a field called\n"
"`Files-Excluded-<component>`. The `<component>` part should then match the component name\n"
"exactly (case sensitive).\n"

This doesnt really work in isolation, what is a <componant>?

Maybe if it just gave an example in passing it would be clear enough,
but I can't find any examples.

Please see the draft above for my proposal for how to resolve this.

I did a similar one for `Files-Included`/`Files-Included-foo`.

#. [Synopsis] One-line description for the value "allow-stderr" [Plaintext].
#. Shown with completion (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
msgctxt "Stanza:Tests|Field:Restrictions"
msgid "Output on standard error should not trigger a test failure"
                                   ^^^^^^
                                   "will not", surely

The flag indicates that this test may routinely log to stderr, and
that the testing frameword should allow this.  It's odd that we call
this a "restriction", but probably it's best not to think about it
too hard.

Indeed.

There I am constrained by the feature ("Restriction") being defined by `autopkgtests`, so it is not in my place to change this. I suspect the mental model they have is that it is a restriction applied to the test runner/testbed rather than the test.

#. [Hover Doc] Extended description for the value "breaks-testbed" [Markdown].
#. Shown in hover docs (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
#, python-brace-format
msgctxt "Stanza:Tests|Field:Restrictions"
[...]
"When this restriction is present the test will usually be skipped\n"
"unless the testbed's virtualisation arrangements are sufficiently\n"

Wait, did I miss that en_GB -isation?  I think so.


You did! :)

"powerful, or alternatively if the user explicitly requests.\n"
msgstr ""

It would be really helpful to say what sbuild does here, eg i think it
mostly skips such tests unless you use lxc?

(No idea)

It would skip the test indeed. As for what constitutes "sufficiently powerful" that I have no answer to. Most of these texts are copy-paste from the documentation of `autopkgtests`.

Side remark: I am not sure `sbuild` is guaranteed to be used. As I recall, the `autopkgtests` supports other backends than `sbuild`. That being said, I have not been following that part of `autopkgtests` or `ci.debian.net` very closely.

#. [Hover Doc] Extended description for the value "flaky" [Markdown]. Shown in
#. hover docs (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
#, python-brace-format
msgctxt "Stanza:Tests|Field:Restrictions"
msgid ""
"The test is expected to fail intermittently, and is not suitable for\n"
"gating continuous integration. This indicates a bug in either the\n"
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    what on earth does this mean?

A piece of continually grating jargonisation, but basically
 https://devops.stackexchange.com/questions/10600/what-is-a-gating-continuous-integration-ci-system#10632
a test that must be passed before the new version is allowed in.


Thanks for pointing that one out. I have made the jargon a link to that stackexchange in the lack of a better solution. The text itself comes directly from `autopkgtests` own documentation, so I am hesitant to stray too far from it without having a textual change that also works for upstream.

(With the added joy of `autopkgtests` using en_GB and `debputy` using en_US. What could possibly go wrong?)

#. [Synopsis] One-line description for the value "isolation-container"
#. [Plaintext]. Shown with completion (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
msgctxt "Stanza:Tests|Field:Restrictions"
msgid "Container level isolation required"
msgstr ""

Can you say what this needs - chroot? schroot? lxc? docker? quemu? it's
really hard to understand from other docs so this would be a great
benefit to users

(Can't help here either)


I cannot help either here.

But I will forward the question to autopkgtests and hear if they can help me provide a better synopsis for this case.

"Tests must not assume that a specific init system is in use: a\n"
"dependency such as `systemd-sysv` or `sysvinit-core` does not work\n"
"in practice, because switching the init system often cannot be done\n"
"automatically.

interesting! given systemd is the "default" is it not assumable in
practice?

(Do all these containerisation mechanisms even use an init system?)

I think you cannot assume systemd being available if you request a container level isolation. As Justin implied, containers do not always use an init system.

#. [Synopsis] One-line description for the value "skippable" [Plaintext].
#. Shown with completion (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
msgctxt "Stanza:Tests|Field:Restrictions"
msgid "Test may at runtime decide to be marked as skipped"

"marked as"?? seems unneccessary

A "skippable" test is apparently one that can decide at runtime
whether it's in a position to work as a test or not (maybe it never
works on the sabbath); if not, it declares that it doesn't count as
having been tried - that is, it's marked down as skipped.

Is there a short way of conveying that?


Perhaps, "Test may at runtime decide to be skipped"?

Anyhow, Justin is right that this is about this. There are cases where the test have to run some probing logic to determine whether it would be able to run in full. If not, it will exit with a special exit code (77) to signal that the test was not run at all (that is, neither failure nor success).

msgstr ""

#. [Hover Doc] Extended description for the value "skippable" [Markdown].
#. Shown in hover docs (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
#, python-brace-format
msgctxt "Stanza:Tests|Field:Restrictions"
msgid ""
"The test might need to be skipped for reasons that cannot be\n"
"described by an existing restriction such as isolation-machine or\n"
"breaks-testbed, but must instead be detected at runtime. If the\n"
"test exits with status 77 (a convention borrowed from Automake), it\n"
"will be treated as though it had been skipped. If it exits with any\n"
"other status, its success or failure will be derived from the exit\n"
"status and stderr as usual. Test authors must be careful to ensure\n"
"that `skippable` tests never exit with status 77 for reasons that\n"
"should be treated as a failure.\n"
msgstr ""

Pretty confusing tbh - why is this needed if we can mark tests as skippable?

This *is* marking them as skippable - the CI system doesn't roll dice
to decide which ones to run, it just lets "skippable" tests return an
extra value that doesn't mean "succeeded" or "failed".  It seems an
odd way to do it, but I haven't tried implementing a test framework.


The problem is that the logic needed to decide whether a test should be skipped can become arbitrarily complex. To solve this problem, major test framework enable tests to include their own logic for whether it should be skipped and then provide a way for the test to signal "This run needs to be skipped!". Higher level languages (such as Python) generally provide "failures" with metadata to enable them to distinguish "skip" vs. "test raised an expected error" (failure, show stack trace) vs. "test ran successfully but failed/gave the wrong result" (failure, show invalid result).

However, the autopkgtests tool relies on the exit code of the process to decide whether a test fails. The problem with exit codes is that it is just a number with no metadata attached to it and you cannot tell whether this 77 means "error" (crash), "failure" (wrong result), or "skip" (no test). To avoid assuming 77 always means "skip", the autopkgtests tooling requires the test writer to explicitly opt-in to "77 = skip" semantics, because it requires the test writer to ensure that 77 only occurs when skip was intended.

[...]

#. [Hover Doc] Extended description for the field itself [Markdown]. Shown in
#. hover docs (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
#, python-brace-format
msgctxt "Stanza:Tests|Field:Depends"
msgid ""
"Declares that the specified packages must be installed for the test\n"
"to go ahead. This supports all features of dpkg dependencies, including\n"
"the architecture qualifiers (see\n"
"<https://www.debian.org/doc/debian-policy/ch-relationships.html>),\n"
"plus the following extensions:\n"
"\n"
"`@` stands for the package(s) generated by the source package\n"
"containing the tests; each dependency (strictly, or-clause, which\n"
                                                     ?????????
"may contain `|`s but not commas) containing `@` is replicated\n"
"once for each such binary package, with the binary package name\n"
"substituted for each `@` (but normally `@` should occur only\n"
"once and without a version restriction).\n"

I dont understand this, the last time i used @ it tried to install *all*
binary packages at once, which didnt work

I've never tried, so I can't help with any of this!

The first line says "stands for the package(s)" so it implies that `@` can expand to multiple packages.

With that being said, this text is straight from the horse's own mouth:

https://salsa.debian.org/ci-team/autopkgtest/raw/master/doc/README.package-tests.rst (Look for "Depends: dpkg dependency field syntax")

If you experience problems with it, I recommend you filing a bug against the autopkgtests package with what you experienced. I would only be a middleman slowing down the communication. Unlikely the previous cases, I am less sure this is only a documentation improvement involved.

"`@recommends@` stands for all the packages listed in the\n"
"`Recommends:` fields of all the binary packages mentioned in the\n"
"`debian/control` file. Please note that variables are stripped,\n"
"so if some required test dependencies aren't explicitly mentioned,\n"
"they may not be installed.\n"

dont understand "stripped" in this para


I suspect it is a reference to substitution variables. But I am not sure especially since substitution variables would not be present at the time. I will ask the autopkgtests maintainers to clarify the text.

"\n"
"If no Depends field is present, `Depends: @` is assumed. Note that\n"
"the source tree's Build-Dependencies are *not* necessarily\n"
"installed, and if you specify any Depends, no binary packages from\n"
"the source are installed unless explicitly requested.\n"

or requested via @?


Proposing that clarification to autopkgtests.

#. [Synopsis] One-line description of the field itself [Plaintext]. Shown with
#. completion (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
msgctxt "Stanza:Tests|Field:Comment"
msgid "Name and email of the maintainer or the maintenance team"
msgstr ""

#. [Hover Doc] Extended description for the field itself [Markdown]. Shown in
#. hover docs (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
#, python-brace-format
msgctxt "Stanza:Tests|Field:Comment"
msgid ""
"Comment field to optionally provide additional information. For example, it might quote an e-mail from\n"
"upstream justifying why the combined license is acceptable to the `main` archive, or an explanation of\n"
"how this version of the package has been forked from a version known to be [DFSG]-free, even though the\n"
"current upstream version is not.\n"

Didnt we already have this field before? and this seems to be about licenses not tests?

There are
  msgctxt "Stanza:Header|Field:Comment"
and
  msgctxt "Stanza:License|Field:Comment"
and
  msgctxt "Stanza:Tests|Field:Comment"

but I can't say I follow what this one's about.  Maybe all the
examples are appropriate for the License-stanza version, and we ought
to have different ones for Comment fields in other stanzas?


The `Tests` one was a copy-waste mistake. I have removed it now. Thanks for reporting it.


#. [Hover Doc] Extended description for the field itself [Markdown]. Shown in
#. hover docs (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
#, python-brace-format
msgctxt "Stanza:Tests|Field:Tests"
msgid ""
"This field names the tests which are defined by this stanza, and map\n"
"to executables/scripts in the test directory.

why the / ? are you trying to say that scripts dont need to be +x (i
dont think that is correct)?

In theory I suppose they might be dotted-in bash scripts, but I really
hope not.  I suspect the idea is "executables (whether or not they're
compiled binaries)", but since the point is that it doesn't matter
(and I don't know whether compiled binaries are even likely here) it
should probably just say "executables".

Ok. Will propose a similar simplification to autopkgtests.

"This allows tests to live outside the `debian/` metadata area, so that\n"
"they can more palatably be shared with non-Debian distributions.\n"
msgstr ""

#. [Synopsis] One-line description of the field itself [Plaintext]. Shown with
#. completion (etc.)
#: src/debputy/lsp/data/debian_tests_control_reference_data.yaml
msgctxt "Stanza:Tests|Field:Copyright"
msgid "Provide copyright statements for the package as a whole"
msgstr ""

         Provides

But why is it Stanza:Tests - is this duplication?

Some of the same fields appear in multiple stanzas, so duplication
isn't necessarily bad.  I don't see why this kind of information would
be in this stanza, though...
--
JBR	with qualifications in linguistics, experience as a Debian
	sysadmin, and probably no clue about this particular package

Indeed, this was also a copy-waste mistake. It will be removed. :)

Best regards,
Niels


Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature


Reply to: