Conversation
adombeck
force-pushed
the
UDENG-8984-fix-lintian-issues
branch
8 times, most recently
from
df4a7b9 to
a6d6a7a
Compare
adombeck
force-pushed
the
UDENG-8984-fix-lintian-issues
branch
9 times, most recently
from
ed5afd3 to
dbba07a
Compare
adombeck
force-pushed
the
UDENG-8984-fix-lintian-issues
branch
6 times, most recently
from
3d782b8 to
e27d0a5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1253 +/- ##
==========================================
+ Coverage 80.08% 85.99% +5.91%
==========================================
Files 20 99 +79
Lines 984 6685 +5701
Branches 0 111 +111
==========================================
+ Hits 788 5749 +4961
- Misses 196 880 +684
- Partials 0 56 +56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adombeck
force-pushed
the
UDENG-8984-fix-lintian-issues
branch
3 times, most recently
from
b02a9ed to
a2f8966
Compare
debian/lrc.config
Outdated
| vendor_rust/pin-project/src/lib.rs | ||
| vendor_rust/pin-project/examples/enum-default-expanded.rs | ||
| vendor_rust/pin-project/examples/enum-default.rs | ||
| vendor_rust/pin-project/examples/not_unpin-expanded.rs | ||
| vendor_rust/pin-project/examples/not_unpin.rs |
There was a problem hiding this comment.
Oh, we should be able to just use vendor_rust/pin-project/. Let me try that
There was a problem hiding this comment.
Yeah, but also they can be excluded using debian/copyright's file-excluded, no?
There was a problem hiding this comment.
Oh, we should be able to just use vendor_rust/pin-project/. Let me try that
so that didn't work, even though the comment from /usr/share/lrc/lrc.config says it should:
# Directories identified by trailing slash / # Entire contents will be (recursively) excluded. debian/patches/
I'll report that on the Debian bug tracker
Yeah, but also they can be excluded using debian/copyright's file-excluded, no?
I don't know what you mean. debian/copyright has no "file-excluded" stanza
There was a problem hiding this comment.
I'll report that on the Debian bug tracker
That's https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1128312
There was a problem hiding this comment.
I don't know what you mean. debian/copyright has no "file-excluded" stanza
https://www.debian.org/doc/manuals/debmake-doc/ch08.en.html#files-excluded
Not sure if that applies to uscan only, but in theory it should be to avoid files being included in a tarball and ignore their copyright.
There was a problem hiding this comment.
https://www.debian.org/doc/manuals/debmake-doc/ch08.en.html#files-excluded
Oh, that's interesting. I looked for that stanza in https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/ which doesn't mention it.
There was a problem hiding this comment.
I now read all the resources I could find on this Files-Excluded stanza:
- https://www.debian.org/doc/manuals/debmake-doc/ch08.en.html#files-excluded
- https://manpages.debian.org/unstable/devscripts/uscan.1.en.html
- https://manpages.debian.org/testing/devscripts/mk-origtargz.1.en.html
From what I understand, it's mostly useful when creating a Debian package from an upstream tarball which includes non-DFSG-compliant files. Since we're creating the tarball ourselves, we can just use the tar-ignore option in debian/source/options, which we already use to exclude some files.
However, simply excluding files from tarball breaks the build, because cargo creates a .cargo-checksum.json file for each vendored crate and verifies the checksums during build. That causes it to fail with:
debian cargo wrapper: running subprocess (['env', 'RUST_BACKTRACE=1', '/usr/bin/cargo', '-Zavoid-dev-deps', 'build', '--verbose', '--verbose', '-j16', '--target', 'x86_64-unknown-linux-gnu', '--release'],) {}
error: failed to calculate checksum of: /build/reproducible-path/authd-0.5.7/vendor_rust/pin-project/tests/ui/pin_project/project_replace_unsized.rs
Caused by:
failed to open file `/build/reproducible-path/authd-0.5.7/vendor_rust/pin-project/tests/ui/pin_project/project_replace_unsized.rs`
when a vendored file is not there during build.
I found this page which includes an example that simply removes the .cargo-checksum.json of all vendored crates to avoid the issue:
# Remove the checksum files to allow us to patch the crates to remove extraneous dependencies
for crate in $(CARGO_VENDOR_DIR)/*; do \
sed -i 's/^{"files":.*"package":"\([a-z0-9]\+\)"}$$/{"files":{},"package":"\1"}/' $$crate/.cargo-checksum.json; \
done
However, I think a better way to avoid the issue is to use the exclude-crate-paths option of cargo-vendor-filterer to exclude the unwanted files during vendoring, so that they are not included in the .cargo-checksum.json in the first place.
There was a problem hiding this comment.
I think a better way to avoid the issue is to use the exclude-crate-paths option of cargo-vendor-filterer
I implemented that and it seems to work.
3v1n0
left a comment
3v1n0
left a comment
There was a problem hiding this comment.
I went through commits, so some of my review comments are outdated.
However, in general it looks very good and an huge effort.
I checked the generation script but not into full details, but the final result seems acceptable to me, so let's go with this.
| # With this approach, libnss is not using soname to track compatibility, so this override is safe. | ||
| authd: shared-library-lacks-version usr/lib/*/libnss_authd.so.2 libnss_authd.so | ||
|
|
||
| # Static-Built-Using is a valid field introduced in dpkg 1.21.0 but not yet recognized by lintian |
There was a problem hiding this comment.
I'm unsure if archive admins are happy to support something that is meant for 26.04 with code for 24.04, but it should also not be harmful
There was a problem hiding this comment.
Nice, worth considering adding this to a generic repo, or even ubuntu dev scripts, since we are vendoring more and more these days.
There was a problem hiding this comment.
Wondering also if some of the reuse tools could help here: https://reuse.software/dev/
There was a problem hiding this comment.
I looked into the reuse tool and many other tools, none of which were able to generate correct entries. The reuse tool is only a linter, it can't be used to print the license information it detects.
debian/lrc.config
Outdated
| vendor_rust/pin-project/src/lib.rs | ||
| vendor_rust/pin-project/examples/enum-default-expanded.rs | ||
| vendor_rust/pin-project/examples/enum-default.rs | ||
| vendor_rust/pin-project/examples/not_unpin-expanded.rs | ||
| vendor_rust/pin-project/examples/not_unpin.rs |
There was a problem hiding this comment.
Yeah, but also they can be excluded using debian/copyright's file-excluded, no?
| fi | ||
| allow-sudo: true | ||
| lintian: --fail-on error,warning,info,pedantic | ||
| lintian: --fail-on error,warning,info,pedantic --verbose |
There was a problem hiding this comment.
I hope this does not break the parser for the summary :)
There was a problem hiding this comment.
There was a problem hiding this comment.
I didn't even know we printed markdown to the job summary. The job summary page is not very visible. If you click on a failed job on a PR, you get directly to the logs. You have to click on "Summary" in the top left to get to the summary page from there. Do you usually do that?
There was a problem hiding this comment.
My point is that, in my view, it's not worth spending effort on improving the summary page but instead make the logs more useful, because that's where we look when a job fails
adombeck
force-pushed
the
UDENG-8984-fix-lintian-issues
branch
from
3e2cced to
2b93744
Compare
adombeck
force-pushed
the
UDENG-8984-fix-lintian-issues
branch
from
eec5083 to
c96073c
Compare
|
I removed
(for later reference, the commit was 4a4a9ce) |
By default, our build-debian action runs lintian with `--fail-on error`. We had an upload of authd to universe rejected because of lintian warnings and pedantic messages. We missed those because the CI succeeded even though lintian also reported those issues there.
As indicated by lintian
lintian complained about a file in docs/.sphinx:
P: authd source: source-contains-prebuilt-javascript-object [docs/.sphinx/_static/js/bundle.js]
The docs are online only documentation, there is no reason to include it
in the source deb.
Also adjusts debian/lrc.config and debian/source/lintian-overrides.
The debian/copyright file was updated with the help of
decopy --group-by copyright --output debian/copyright --exclude '.*/snapcraft.yaml|debian/.*'
with my decopy fork [1] and a lot of manual adjustments of the result.
[1] https://salsa.debian.org/adombeck/decopy
If you regenerate the copyright file with decopy, check the following:
* The copyright and license of these entries:
* "Files: *"
* "Files: pam/integration-tests/pam_mkhomedir/pam_mkhomedir.c"
* "Files: pam/internal/gdm/extensions/*"
* "Files: pam/internal/gdm/extensions/gdm-pam-extensions-common.h"
* The "Files: cmd/*" entry should not be removed
* No "Files: vendor/*" entry
* No "Files: vendor_rust/*" entry
* No MIT license, it should be "Expat"
* No BSD or BSD-3 license, it should be "BSD-3-clause"
* No Apache-2.0, it should be "Apache-2"
* No "Comment: Add the corresponding license text here"
* The occurrences of "License: CC0" are probably false positives
* The occurrences of "License: Unlicense" are probably false positives
* Run licenserecon aka lrc
* Rebuild the Debian package and run lintian (both on source and binary
package)
We are seeing weird lintian behavior along with the output N: Some overrides were ignored. N: Use --verbose for more information.
Older lintian versions used the silent-on-rules-requires-root tag to
complain when the "Rules-Requires-Root:" field was missing:
P: authd source: silent-on-rules-requiring-root [debian/control]
That tag was removed in newer lintian versions and replaced with
redundant-rules-requires-root-no-field which checks the opposite, i.e.
that there is *no* "Rules-Requires-Root: no" field. We comply with the
new lintian versions by not setting the field.
To avoid older lintian versions to fail, we ignore the tag by adding it
to debian/source/lintian-overrides. We only do that on Nobel and Plucky
because the tag was removed in later lintian versions which would cause
lintian to fail with:
E: authd source: alien-tag silent-on-rules-requiring-root
All the other tools I tried were not able to generate valid entries in debian/copyright for our vendored dependencies. The tool which came closest was decopy, but it still required a lot of manual effort to fix things after running it (see commit message of commit "debian: Update copyright file") and its output is non-reproducible, so we can't use it in CI to check if the file needs to be regenerated. I took time to implement our own script to generate these entries in debian/copyright for our vendored dependencies.
licensecheck doesn't detect SPDX identifiers so it reports the wrong license for a lot of files with SPDX identifiers.
We now have a way to automatically generate valid entries for vendored files in debian/copyright, so now it's an acceptable effort to keep this file maintained so that licensrecon is happy.
Drop the comma, as in the examples in https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/#copyright-field
Many of our vendored dependencies do not declare a copyright holder.
We now exclude all 'examples' and 'tests' directories from vendored crates via cargo-vendor-filterer, so we don't need to tell lrc anymore to ignore these.
adombeck
force-pushed
the
UDENG-8984-fix-lintian-issues
branch
from
c96073c to
cd2f10e
Compare
The bubblewrap tests fail in autopkgtest even in CI, for example:
===== Running TestSetUserID/Successfully_set_UID_if_ID_is_already_in_use_as_GID_of_authd_user in bubblewrap =====␛[0m
command: /usr/bin/unshare --user --map-root-user --map-users=auto --map-groups=auto /tmp/authd-bwrap-2803473299/bwrap --ro-bind / / --dev /dev --tmpfs /tmp --bind /tmp/TestSetUserIDSuccessfully_set_UID_if_ID_is_already_in_use_as_GI585098413/001/etc /etc --ro-bind /etc/environment /etc/environment --ro-bind /etc/localtime /etc/localtime --ro-bind /etc/login.defs /etc/login.defs --ro-bind /etc/nsswitch.conf /etc/nsswitch.conf --ro-bind /etc/sudo.conf /etc/sudo.conf --ro-bind /etc/sudoers /etc/sudoers --ro-bind-try /etc/timezone /etc/timezone --ro-bind /etc/pam.d /etc/pam.d --ro-bind /etc/security /etc/security --bind /tmp/go-build33275020/b503/users.test /tmp/go-build33275020/b503/users.test /tmp/go-build33275020/b503/users.test -test.run ^TestSetUserID/Successfully_set_UID_if_ID_is_already_in_use_as_GID_of_authd_user$
----------------------------------------
environment: [PATH=PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin BUBBLEWRAP_TEST=1 TESTS_UPDATE_GOLDEN=]
----------------------------------------
18:26:10 DEBUG Loading SQLite database from /testdata/db/multiple_users_and_groups.db.yaml
--- FAIL: TestSetUserID (0.00s)
--- FAIL: TestSetUserID/Successfully_set_UID_if_ID_is_already_in_use_as_GID_of_authd_user (0.00s)
manager_bwrap_test.go:90:
Error Trace: /tmp/autopkgtest.8HEg0B/build.0X4/src/internal/users/manager_bwrap_test.go:90
Error: Received unexpected error:
open /testdata/db/multiple_users_and_groups.db.yaml: no such file or directory
Test: TestSetUserID/Successfully_set_UID_if_ID_is_already_in_use_as_GID_of_authd_user
Messages: Setup: could not create database from testdata
Another example:
===== Running TestChownRecursiveFrom/Successfully_change_owner in bubblewrap =====␛[0m
command: /usr/bin/unshare --user --map-root-user --map-users=auto --map-groups=auto /tmp/authd-bwrap-2291655303/bwrap --ro-bind / / --dev /dev --tmpfs /tmp --bind /tmp/TestChownRecursiveFromSuccessfully_change_owner2712748225/001/etc /etc --ro-bind /etc/environment /etc/environment --ro-bind /etc/localtime /etc/localtime --ro-bind /etc/login.defs /etc/login.defs --ro-bind /etc/nsswitch.conf /etc/nsswitch.conf --ro-bind /etc/sudo.conf /etc/sudo.conf --ro-bind /etc/sudoers /etc/sudoers --ro-bind-try /etc/timezone /etc/timezone --ro-bind /etc/pam.d /etc/pam.d --ro-bind /etc/security /etc/security --bind /tmp/go-build33275020/b471/fileutils.test /tmp/go-build33275020/b471/fileutils.test /tmp/go-build33275020/b471/fileutils.test -test.run ^TestChownRecursiveFrom/Successfully_change_owner$
----------------------------------------
environment: [PATH=PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin BUBBLEWRAP_TEST=1 TESTS_UPDATE_GOLDEN=]
----------------------------------------
--- FAIL: TestChownRecursiveFrom (0.00s)
--- FAIL: TestChownRecursiveFrom/Successfully_change_owner (0.01s)
fileutils_test.go:534: ChownRecursiveFrom error: <nil>
fileutils_test.go:561:
Error Trace: /tmp/autopkgtest.8HEg0B/build.0X4/src/internal/testutils/golden/golden.go:254
/tmp/autopkgtest.8HEg0B/build.0X4/src/internal/testutils/golden/golden.go:99
/tmp/autopkgtest.8HEg0B/build.0X4/src/internal/fileutils/fileutils_test.go:561
Error: Received unexpected error:
open /testdata/golden/TestChownRecursiveFrom/Successfully_change_owner: no such file or directory
Test: TestChownRecursiveFrom/Successfully_change_owner
Messages: Cannot read golden file /testdata/golden/TestChownRecursiveFrom/Successfully_change_owner
adombeck
force-pushed
the
UDENG-8984-fix-lintian-issues
branch
from
cd2f10e to
87b9833
Compare
2 participants
No description provided.