[MIR] mdevctl 1.0.0 (rust switch)

Hi,
@ubuntu-archive - since [1] is a junk repo I can't propose an MR to it.
I subscribed you to please merge the attached diff to sync-blacklist.txt to not make this even more pain by auto-syncing 1.0 over once 22.04 opens up.

[1]: https://bazaar.launchpad.net/~ubuntu-archive/+junk/sync-blacklist/view/head:/sync-blacklist.txt

Done, so now at least we won't get it auto-synced accidentally in the near future.

Thank you Lukasz, I have also contacted Matt/Steve to get a status/ETA on rust-in-main overall.
There isn't much to share yet nor something we could link this bug to :-/

FYI I'm sure more will come e.g. virtiofsd-rs https://virtio-fs.gitlab.io/

To get a sense of the scale despite mdevctl being so small in it's function here
the rust portion of the build-dependency tree. (Due to the way rust libs work some are from the same source - so two lists):

Maybe some of them are for dh-rust, maybe we need all of them. Since there is no full detection for built-using yet (that I'd know of) this is guessing from what I get when I run apt build-dep in an empty container.

Sources to MIR:
Source: rust-aho-corasick
Source: rust-ansi-term
Source: rust-anyhow
Source: rust-atty
Source: rust-autocfg
Source: rust-bitflags
Source: rust-cfg-if-0.1
Source: rust-clap
Source: rust-cloudabi (0.0.3-1)
Source: rust-env-logger
Source: rust-getrandom
Source: rust-heck
Source: rust-humantime
Source: rust-indexmap
Source: rust-itoa
Source: rust-lazy-static
Source: rust-libc
Source: rust-lock-api
Source: rust-log
Source: rust-memchr
Source: rust-once-cell
Source: rust-parking-lot
Source: rust-parking-lot-core
Source: rust-ppv-lite86
Source: rust-proc-macro-error
Source: rust-proc-macro-error-attr
Source: rust-proc-macro2
Source: rust-quote
Source: rust-rand
Source: rust-rand-chacha
Source: rust-rand-core
Source: rust-rand-hc (0.2.0-1)
Source: rust-redox-syscall
Source: rust-regex
Source: rust-regex-syntax
Source: rust-remove-dir-all
Source: rust-ryu
Source: rust-scopeguard
Source: rust-serde
Source: rust-serde-json
Source: rust-smallvec
Source: rust-strsim
Source: rust-structopt
Source: rust-structopt-derive
Source: rust-syn
Source: rust-syn-mid
Source: rust-tempfile
Source: rust-termcolor
Source: rust-textwrap (0.11.0-1)
Source: rust-thread-local
Source: rust-unicode-segmentation
Source: rust-unicode-width
Source: rust-unicode-xid
Source: rust-uuid
Source: rust-vec-map (0.8.1-2)
Source: rust-version-check
Source: rust-winapi
Source: rust-winapi-i686-pc-windows-gnu (0.4.0-1)
Source: rust-winapi-util
Source: rust-winapi-x86-64-pc-windows-gnu (0.4.0-1)

Effective full build-time dependencies:
librust-aho-corasick+std-dev
librust-aho-corasick-dev
librust-ansi-term-dev
librust-anyhow-dev
librust-atty-dev
librust-autocfg-dev
librust-bitflags-dev
librust-cfg-if-0.1-dev
librust-clap+color-dev
librust-clap+default-dev
librust-clap+strsim-dev
librust-clap-dev
librust-cloudabi+default-dev
librust-cloudabi-dev
librust-env-logger+default-dev
librust-env-logger-dev
librust-getrandom-dev
librust-heck-dev
librust-humantime-dev
librust-indexmap-dev
librust-itoa-dev
librust-lazy-static-dev
librust-libc-dev
librust-lock-api-dev
librust-log+serde-dev
librust-log-dev
librust-memchr-dev
librust-once-cell-dev
librust-parking-lot-core-dev
librust-parking-lot-dev
librust-ppv-lite86-dev
librust-proc-macro-error-attr-dev
librust-proc-macro-error-dev
librust-proc-macro2-dev
librust-quote+proc-macro-dev
librust-quote-dev
librust-rand+alloc-dev
librust-rand+getrandom-dev
librust-rand+std-dev
librust-rand-chacha+std-dev
librust-rand-chacha-dev
librust-rand-core+getrandom-dev
librust-rand-core+std-dev
librust-rand-core-dev
librust-rand-dev
librust-rand-hc-dev
librust-redox-syscall-dev
librust-regex+default-dev
librust-regex+perf-cache-dev
librust-regex+perf-dev
librust-regex+perf-literal-dev
librust-regex+unicode-age-dev
librust-regex+unicode-b...

FYI (mostly a note to myself I guess) about dynamic rust:

Along the way in I found that rust seems to have dynamic libraries - see
"dylib/cdylib" in [2].
[1] shows -C prefer-dynamic target-feature=-crt-static" which could be our
usual build style for a distro making dependencies and code-resuage through
dynamic libs "normal".

But before I could cheer I found that:
- One argument against it that I found is that two dynamic libs that use the
  same (static) rlib can not work together. So it would have to be make all
  or nothing dynamic. (That would be ok I guess)
- Another I've quckly found is that ABI isn't fixed and thereby a compiler change
  between a rebuild of either component could break things. (This breaks the
  camels back)

[1]: https://rust-lang.github.io/rfcs/0404-change-prefer-dynamic.html
[2]: https://doc.rust-lang.org/reference/linkage.html

dh_golang adds misc:Built-Using via `go list -f`.
1. on the target binaries, deriving "{{ range .Deps }}".
2. And then from all Deps deriving the filenames
3. and then it tracks down the package that owns the files
=> That then makes the list for Built-Using

That does not seem to exist for Rust yet.

The build-cache [1] has plenty of information on the build.
Hopefully we might be able to derive somiething from there.
rlibs are part of the linking, think intermediate static
libs [2][3].

Gladly I found that I do not have to re-invent the wheel.
dh-cargo-built-using does what I'd expect.

Along cargo install it dh_cargo runs this already.
The current format in d/control for this seems to be

  Built-Using: ${cargo:Built-Using}
  XB-X-Cargo-Built-Using: ${cargo:X-Cargo-Built-Using}

That might follow the debate in golang to not use Built-Using for
the libraries as it already splits compiler (still useful for the
potential ABI issues) but puts the libs in an extra non-standard field.

I've added this to mdevctl so that we can continue to use this as
test-example for potential e.g. britney extensions to probe component
mismatches based on it.

[1]: https://doc.rust-lang.org/cargo/guide/build-cache.html
[2]: https://doc.rust-lang.org/reference/linkage.html
[3]: https://rustc-dev-guide.rust-lang.org/backend/libs-and-metadata.html

With the above I come down further to "just" 36 libraries.
The rest is only indirectly, not effectively or build-time used.

I've not uploaded this to Debian yet as we also have nothing on our end
that would evaluate/block on it.

rust-aho-corasick (= 0.7.10-1)
rust-ansi-term (= 0.12.1-1)
rust-anyhow (= 1.0.31-1)
rust-atty (= 0.2.14-2)
rust-bitflags (= 1.2.1-1)
rust-cfg-if-0.1 (= 0.1.10-2)
rust-clap (= 2.33.3-1)
rust-env-logger (= 0.7.1-2)
rust-getrandom (= 0.1.13-4)
rust-humantime (= 2.0.0-1)
rust-indexmap (= 1.3.2-1)
rust-itoa (= 0.4.3-1)
rust-lazy-static (= 1.4.0-1)
rust-libc (= 0.2.80-1)
rust-log (= 0.4.11-2)
rust-memchr (= 2.3.3-1)
rust-once-cell (= 1.5.2-1)
rust-ppv-lite86 (= 0.2.6-2)
rust-rand-chacha (= 0.2.2-1)
rust-rand-core (= 0.5.1-1)
rust-rand (= 0.7.3-3)
rust-regex (= 1.3.7-1)
rust-regex-syntax (= 0.6.17-1)
rust-remove-dir-all (= 0.5.2-1)
rust-ryu (= 1.0.2-1)
rust-serde (= 1.0.106-1)
rust-serde-json (= 1.0.41-1)
rust-strsim (= 0.9.3-1)
rust-structopt (= 0.3.20-1)
rust-tempfile (= 3.1.0-1)
rust-termcolor (= 1.1.0-1)
rust-textwrap (= 0.11.0-1)
rust-thread-local (= 1.1.3-3)
rust-unicode-width (= 0.1.8-1)
rust-uuid (= 0.8.1-3)
rust-vec-map (= 0.8.1-2)
rustc (= 1.49.0+dfsg1-1)

As a hint that really more things in main might come up using it, here a follow up to the KVM Forum 2021.
=> https://wiki.qemu.org/RustInQemu

 mdevctl | 0.81-1 | jammy
 mdevctl | 0.81-1 | kinetic

mdevctl | 0.81-1 | stable
mdevctl | 1.1.0-1 | testing
mdevctl | 1.1.0-1 | unstable
mdevctl | 1.1.0-1 | unstable-debug

Rust MIR rules landed, this is ready for review.
Un-assigning Athos to be MIR-Team visible.

P.S. Review from the PPA or MP as described, because due to the nature of how propose migration works this would "just migrate" if we upload it as-is.

Review for Package: src:mdevctl

[Summary]
This is the 1st Rust package to be MIRed, using newly adopted MIR rules.
The package looks rather clean and under control, except for all of the vendored
dependencies/sources, which can be overwhelming. But we have the Sever team's
written consent to stay on top of them for the lifetime of the package.
Lot's of "Recommended TODOs" have been stated below, which should be improved
upon step by step, but nothing critical blocking the promotion IMO.

MIR team ACK

This does need a security review, so I'll assign ubuntu-security
That is because it's 1st the Rust package and we want the security-team's input
on this. Doesn't seem super security senstive otherwise (maybe the vendored libc
bindings).

List of specific binary packages to be promoted to main: mdevctl
Specific binary packages built, but NOT to be promoted to main: <None>

Notes:
- This is the 1st Rust package to be MIRed, using newly adopted MIR rules.
- embedded source present
- static linking in use
- handles data formats (JSON output) via serde-rs JSON library

Required TODOs:
- None

Recommended TODOs:
#1:
- dh_auto_test does not recognize the 'nocheck' DEB_BUILD_OPTION
  I: mdevctl source: override_dh_auto_test-does-not-check-DEB_BUILD_OPTIONS [debian/rules:22]
  => This should be fixed according to lintian:
     ifeq (,$(filter nocheck,$(DEB_BUILD_OPTIONS)))

#2:
- does NOT have a non-trivial test suite that runs as autopkgtest
  => as lined out in the bug description, this should be fixed mid/long term.

#3.1: Lintian warnings/errors, some that feel worth mentioning:
- E: mdevctl source: unpack-message-for-orig mdevctl_1.1.0.orig-vendor.tar.gz . ar failed for vendor/winapi-i686-pc-windows-gnu/lib/*.a
- E: mdevctl source: unpack-message-for-orig mdevctl_1.1.0.orig-vendor.tar.gz . ar failed for vendor/winapi-x86_64-pc-windows-gnu/lib/*.a
  => Seems to be OK, as those are only Windows libraries, unused in Ubuntu.

#3.2:
- I: mdevctl source: out-of-date-standards-version 4.6.0 (released 2021-08-18) (current is 4.6.1.0)
  => can be fixed with the next version bump

#3.3:
- P: mdevctl source: very-long-line-length-in-source-file
  => lots of those, maybe they should be muted.
     It's false positives as it is complaining about .a windows binaries

#3.4: (some lintian recommendations)
  X: mdevctl source: prefer-uscan-symlink filenamemangle s/.+\/v?(\d\S+)\.tar\.gz/mdevctl-$1\.tar\.gz/ [debian/watch:3]
  X: mdevctl source: upstream-metadata-file-is-missing

#4: upstream build-time warnings (in vendored code). could be double checked
    and reported to the upstream developers
- warnings during the build (vendorized deps); some deprecations
  warning: `ansi_term` (lib) generated 12 warnings
  warning: `vec_map` (lib) generated 3 warnings
  warning: `log` (lib) generated 1 warning
  warning: `regex-syntax` (lib) generated 2 warnings
  warning: `aho-corasick` (lib) generated 3 warnings
  warning: `clap` (lib) generated 77 warnings
  warning: `proc-macro-error` (lib) generated 1 warning

[Duplication]
There is no other package in main providing the same functionality.
Except the previous (non-rust) version of mdevctl itself.

[Dependencies]
OK:
-...

I updated the PPA and the MP to perform the changes on top of the new version available in Debian (1.2.0). It also addresses some of the recommended TODOs pointed by Lukas.

I filed an FFe bug in LP: #1987551 so we can upload this to kinetic once it's complete.

There's more build warnings than I usually like to see:

$ grep warning: mdevctl_1.1.0-1ubuntu1~ppa2_amd64-2022-08-09T15\:02\:28Z.build | sort | uniq -c | sort -nr
    126 warning: trait objects without an explicit `dyn` are deprecated
     88 = warning: this is accepted in the current edition (Rust 2015) but is a hard error in Rust 2021!
     28 = warning: this is accepted in the current edition (Rust 2015) but is a hard error in Rust 2021!
     14 warning: unnecessary parentheses around block return value
     10 = warning: this is accepted in the current edition (Rust 2015) but is a hard error in Rust 2021!
      9 warning: panic message is not a string literal
      8 warning: use of deprecated associated function `std::error::Error::description`: use the Display impl or to_string()
      8 warning: use of deprecated associated function `bitflags::core::str::<impl str>::trim_left_matches`: superseded by `trim_start_matches`
      6 warning: unnecessary trailing semicolon
      6 warning: unnecessary parentheses around type
      4 = warning: this is accepted in the current edition (Rust 2015) but is a hard error in Rust 2018!
      4 warning: anonymous parameters are deprecated and will be removed in the next edition
      3 dpkg-scanpackages: warning: Packages in archive but missing from override file:
...

some of these are obviously a waste of time but some of them might represent challenges backporting fixes from the future.

Hi Seth,

Thanks for the feedback here!

As mentioned in https://bugs.launchpad.net/ubuntu/+source/mdevctl/+bug/1942394/comments/14, we updated the package in the PPA (https://launchpad.net/~athos-ribeiro/+archive/ubuntu/mdevctl-vendored-plus-lockfile/+packages) to the new version available in Debian (1.2.0).

This new version fixed the majority of the warnings we were seeing in 1.1.0.

Here is the new output for your grep snippet:

$ grep warning: buildlog_ubuntu-kinetic-amd64.mdevctl_1.2.0-1ubuntu1~ppa1_BUILDING.txt | sort | uniq -c | sort -nr
      2 warning: unknown lint: `unused_macro_rules`
      2 warning: `hashbrown` (lib) generated 1 warning
      1 warning: `proc-macro-error` (lib) generated 1 warning
      1 warning: panic message is not a string literal
      1 warning: field is never read: `start`
      1 warning: field is never read: `last_match_end`
      1 warning: field is never read: `config`
      1 warning: be sure to add `debian/mdevctl/usr/bin` to your PATH to be able to run the installed binaries
      1 warning: `aho-corasick` (lib) generated 3 warnings
      1 dpkg: warning: unable to delete old directory '/usr/lib/udev': Directory not empty
      1 dpkg-source: warning: cannot verify signature ./mdevctl_1.2.0-1ubuntu1~ppa1.dsc
      1 dpkg-gencontrol: warning: Suggests field of package mdevctl: substitution variable ${cargo:Suggests} used, but is not defined
      1 dpkg-gencontrol: warning: Recommends field of package mdevctl: substitution variable ${cargo:Recommends} used, but is not defined
      1 dpkg-gencontrol: warning: Provides field of package mdevctl: substitution variable ${cargo:Provides} used, but is not defined
      1 dpkg-gencontrol: warning: Depends field of package mdevctl: substitution variable ${cargo:Depends} used, but is not defined

Now, there are 2 different warnings being raised there:

- warning: panic message is not a string literal
This is related to this Rust 2021 change: https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html

kinetic ships with rustc 1.59.0, where this warning is not an error. Starting from rustc 1.60.0, this becomes an error and we will need to change that panic!(AbortNow) call to std::panic::panic_any(AbortNow). Since this warning is being issued for a vendorized dependency, I wonder if we should patch it right away and forward a patch upstream, or just patch it upstream and introduce the Delta when/if it is not fixed by the time rustc >= 1.60.0 lands (this would be a delta since the debian package does not vendorize the dependency).

- warning: field is never read: `start`
This is a warning for possible dead code detected by the compiler which could be dealt with upstream in mid/long term.

Finally, a new version of a BuildDep landed in Debian unstable and kinetic-proposed (not migrated yet by the time of this writing). rust-uuid (0.8.1-4 to 1.1.2-2) .

This changes some binary buildDependencies (not used for our build anyway) and leads to FTBFS.
I will forward a fix to Debian and change introduce it as a delta here. Would you also like me to introduce the delta to silence the warning mentioned above?

Thanks!

I just pushed a delta to fix the FTBFS issue (mdevctl - 1.2.0-1ubuntu1~ppa2).

For the Debian package, the new dependency is incompatible with mdevctl, so we will need to work further with upstream to add support for the new uuid version in unstable (this should not be an issue here ATM since that dependency is vendorized in this proposal).

https://launchpad.net/~athos-ribeiro/+archive/ubuntu/mdevctl-vendored-plus-lockfile/+packages
https://code.launchpad.net/~athos-ribeiro/ubuntu/+source/mdevctl/+git/mdevctl/+merge/425351

A fix for the Debian package FTBFS is available at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1017369.

I proposed an upstream fix at https://github.com/mdevctl/mdevctl/pull/67.

We are convinced all that was raised so far is addressed in the new version we already have.
We added block-proposed to have it in kinetic-proposed and stop the PPA indirection.

Expect an update by Athos once it is there.

As mentioned in the previous comment, we blocked the package from migrating with the "block-proposed" tag in this bug and uploaded it [1].

Once this review process is complete, and if it happens in the next 1 week, we can just remove the tag and let the package migrate. Otherwise, we may want to re-visit the FFe bug at LP: #1987551 to ensure this still acceptable for kinetic.

[1] https://launchpad.net/ubuntu/+source/mdevctl/1.2.0-1ubuntu1

I reviewed mdevctl 1.1.0-1ubuntu1~ppa2 as checked into kinetic. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

mdevctl is a tool to manipulate the Linux kernel's mediated devices
framework ('sub devices' intended for use by virtual machines). This rust
rewrite replaces an earlier shell script. The primary consumer is going to
be sysadmin and sysadmin tooling: it shouldn't itself be part of a
privilege boundary.

This package is an ideal test candidate for Rust-in-main -- it's replacing
a shell script, which is a huge improvement no matter how you look at it.
It's not on a privilege boundary itself, so we're unlikely to have real
problems with this specific tool. It's small enough to be approachable. It
vendors in half the rust ecosystem, so we'll have an opportunity to test
our tooling.

- CVE History:
  - No CVEs
- Build-Depends?
  - No cryptography.
  cargo,
  debhelper-compat (= 13),
  dh-cargo,
  librust-anyhow-dev (>= 1.0.0),
  librust-env-logger+default-dev,
  librust-log+serde-dev,
  librust-serde-json+indexmap-dev,
  librust-clap-derive-dev (>= 3.0.7),
  librust-clap-complete-dev (>= 3.0.6),
  librust-clap+strsim-dev,
  librust-tempfile-dev (>= 3.1.0) <!nocheck>,
  pkg-config,
  python3-docutils,
  systemd,
  udev,
  PLUS direct Cargo.toml dependencies: anyhow, clap, env_logger, log,
  serde_json, uuid, tempfile
- pre/post inst/rm scripts?
  None
- init scripts?
  None
- systemd units?
  None
- dbus services?
  None
- setuid binaries?
  None
- binaries in PATH?
  /usr/bin/mdevctl and /usr/bin/lsmdev
- sudo fragments?
  None
- polkit files?
  None
- udev rules?
  Yes, runs /usr/sbin/mdevctl start-parent-mdevs on two events; probably
  safe
- unit tests / autopkgtests?
  Tests are run during the build; depth not checked
- cron jobs?
  None
- Build logs:
  Clean

- Processes spawned?
  Processes spawned in src/callouts.rs, looked safe; script under control
  of admin
- Memory management?
  glorious glorious rust!
- File IO?
   Configuration files read, control files read and written; there's a
   neat testing infrastructure that probably helps
- Logging?
  glorious glorious rust!
- Environment variable usage?
  Not in the main body; probably not in vendored content, but hard to know
- Use of privileged functions?
  Writes through /sys/bus/mdev/devices and /sys/class/mdev_bus
- Use of cryptography / random number sources etc?
  UUIDs, looks fine
- Use of temp files?
  Not directly
- Use of networking?
  None
- Use of WebKit?
  None
- Use of PolicyKit?
  None
- Any significant cppcheck results?
  None
- Any significant Coverity results?
  None
- Any significant shellcheck results?
  None
- Any significant bandit results?
  None

mdevctl feels a bit rough as Rust code goes. It feels like the first large
project of the author, and Rust idioms aren't quite fully ingrained into
this codebase. Unfortunately I'm not skilled enough with Rust myself to
have concrete recommendations for making it feel more like other Rust
programs, besides a suggestion that stronger use of Option, Iterators,
Into, could simplify a lot of the code. I'd *love* to watch along someone
refactoring this program.

That said, this ...

Thank you @Seth!
Thereby everything is complete.
Since it is already in main and the upload in proposed there is not much to do.
I have removed the block-proposed tag and after beta freeze is lifted the new rust based version should migrate to -release.

Thanks, Seth, Christian!

It seems that the bit regarding package migration blockage described in the freeze process in https://wiki.ubuntu.com/BetaProcess only applies for seeded packages. Therefore, the package migrated.

In that sense it is seeded, as libvirt is and that pulls it in.
I'm afraid the reason is that this would hold things in -unapproved but we have passed that long ago staying in -proposed already for many weeks.

Anyway - if I had thought more about these subtle details upfront I'd have held back but now it happened and we can't change it anymore. I hope it isn't too much of a problem for anyone.