Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Debian-based system builds #898

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Add Debian-based system builds #898

merged 3 commits into from
Sep 4, 2024

Conversation

Kreeblah
Copy link
Contributor

@Kreeblah Kreeblah commented Sep 2, 2024

What does this PR do

This adds builds for Debian-based systems (both x86_64 and ARM64).

It ended up being easier than I expected. Just adding some bits to Cargo.toml and then modified the GitHub release actions to use them. I don't know whether this is necessarily the best way to do it (I'm not 100% familiar with GitHub actions), but this at least does generate appropriate packages. There are a couple of caveats here, though.

First, I wasn't sure what to put down for the maintainer or copyright, so those are dummy values. They probably should be updated.

Second, it seems that the binaries I'm generating for x86_64 are panicking, but only when generated from GitHub actions when I was testing (running the same things locally generates binaries that work fine, and the binaries already released from v15.0.0 in this project work fine as well). This doesn't seem to be related to the packaging process, since it's happening even with the binary in the gzipped tarball that gets generated. Specifically, the error I'm getting is:

The application panicked (crashed).
Message:  called `Option::unwrap()` on a `None` value
Location: src/config.rs:910

But, as mentioned, it seems to work fine when I build on my own machines manually or with the pre-built binaries here in GitHub or when creating ARM64 builds from GitHub actions. Those all work. So, if it's something that's replicable, it might be worth looking into.

Standards checklist

  • The PR title is descriptive.
  • I have read CONTRIBUTING.md
  • Optional: I have tested the code myself

For new steps

  • Optional: Topgrade skips this step where needed
  • Optional: The --dry-run option works with this step
  • Optional: The --yes option works with this step if it is supported by
    the underlying command

If you developed a feature or a bug fix for someone else and you do not have the
means to test it, please tag this person here.

@SteveLauC
Copy link
Member

SteveLauC commented Sep 3, 2024

First, I wasn't sure what to put down for the maintainer or copyright, so those are dummy values. They probably should be updated.

If you are willing to help, you can write your email there, I am currently not a Debian user, so if there is anything related to the distro, I cannot help😢.

Second, it seems that the binaries I'm generating for x86_64 are panicking,

Seems like a bug, thanks for catching it, I will give it a fix!

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on it, mostly nits:)

@SteveLauC
Copy link
Member

SteveLauC commented Sep 3, 2024

Second, it seems that the binaries I'm generating for x86_64 are panicking,

Seems like a bug, thanks for catching it, I will give it a fix!

Fixed in #899

@Kreeblah
Copy link
Contributor Author

Kreeblah commented Sep 3, 2024

First, I wasn't sure what to put down for the maintainer or copyright, so those are dummy values. They probably should be updated.

If you are willing to help, you can write your email there, I am currently not a Debian user, so if there is anything related to the distro, I cannot help😢.

I can help, sure. I'm not familiar with Rust (picking it up is on my to-do list, but I'm also not sure when I'll have time to really devote to it), but I am familiar with Debian. So, yeah, if you want, I can do that and check out any packaging-related issues that come up. I don't expect there to be too many.

Second, it seems that the binaries I'm generating for x86_64 are panicking,

Seems like a bug, thanks for catching it, I will give it a fix!

Ah, thank you!

@Kreeblah
Copy link
Contributor Author

Kreeblah commented Sep 3, 2024

OK, I addressed most of the feedback there. Would you like me to remove the -p option from the mkdir commands as well? If so, I can do that, too.

Also, while I was going through this, I remembered one other question I wanted to ask. There was already a [package.metadata.deb] section here from some prior contribution, and it specified the git package as a runtime dependency. I don't really see why that would be, honestly. It seems to run just fine without git installed, so would you have any objections to me removing that as a listed dependency?

@SteveLauC
Copy link
Member

So, yeah, if you want, I can do that and check out any packaging-related issues that come up. I don't expect there to be too many.

Thanks!

There was already a [package.metadata.deb] section here from some prior contribution, and it specified the git package as a runtime dependency. I don't really see why that would be, honestly. It seems to run just fine without git installed, so would you have any objections to me removing that as a listed dependency?

We should remove it, please go ahead, thanks for catching it!


BTW, can we upload the built binaries to the official Debian repo, is it feasible? There is a tracking issue for this

@Kreeblah
Copy link
Contributor Author

Kreeblah commented Sep 4, 2024

There was already a [package.metadata.deb] section here from some prior contribution, and it specified the git package as a runtime dependency. I don't really see why that would be, honestly. It seems to run just fine without git installed, so would you have any objections to me removing that as a listed dependency?

We should remove it, please go ahead, thanks for catching it!

Done. I just pushed another commit with that removed as a dependency.

BTW, can we upload the built binaries to the official Debian repo, is it feasible? There is a tracking issue for this

I'll need to look into whether they accept packages that can't be built with available Debian packages. If they do, then there's actually one more change (that I know of, anyway) which would need to happen for it to be submitted and isn't currently addressed here. I didn't do this bit because I'm not quite sure what the best way would be to set it up (and, unless the package is being submitted to the Debian repo, probably doesn't matter to much).

What would need to be added is the changelog. Essentially, it's an ever-growing text file in a specific format detailing the changes for each release. The specific format is located at https://manpages.debian.org/testing/dpkg-dev/deb-changelog.5.en.html and an example of this is from the openssh package:

openssh (1:9.2p1-2+deb12u3) bookworm-security; urgency=high

  * Non-maintainer upload by the Security Team.
  * Disable async-signal-unsafe code from the sshsigdie() function

 -- Salvatore Bonaccorso <[email protected]>  Sat, 22 Jun 2024 21:38:08 +0200

openssh (1:9.2p1-2+deb12u2) bookworm-security; urgency=medium

  * Cherry-pick from upstream:
    - [CVE-2023-28531] ssh-add(1): when adding smartcard keys to
      ssh-agent(1) with the per-hop destination constraints (ssh-add -h ...)
      added in OpenSSH 8.9, a logic error prevented the constraints from
      being communicated to the agent. This resulted in the keys being added
      without constraints. The common cases of non-smartcard keys and keys
      without destination constraints are unaffected. This problem was
      reported by Luci Stanescu (closes: #1033166).
    - [CVE-2023-48795] ssh(1), sshd(8): implement protocol extensions to
      thwart the so-called "Terrapin attack" discovered by Fabian Bäumer,
      Marcus Brinkmann and Jörg Schwenk. This attack allows a MITM to effect
      a limited break of the integrity of the early encrypted SSH transport
      protocol by sending extra messages prior to the commencement of
      encryption, and deleting an equal number of consecutive messages
      immediately after encryption starts. A peer SSH client/server would
      not be able to detect that messages were deleted.
    - [CVE-2023-51384] ssh-agent(1): when adding PKCS#11-hosted private keys
      while specifying destination constraints, if the PKCS#11 token
      returned multiple keys then only the first key had the constraints
      applied. Use of regular private keys, FIDO tokens and unconstrained
      keys are unaffected.
    - [CVE-2023-51385] ssh(1): if an invalid user or hostname that contained
      shell metacharacters was passed to ssh(1), and a ProxyCommand,
      LocalCommand directive or "match exec" predicate referenced the user
      or hostname via %u, %h or similar expansion token, then an attacker
      who could supply arbitrary user/hostnames to ssh(1) could potentially
      perform command injection depending on what quoting was present in the
      user-supplied ssh_config(5) directive. ssh(1) now bans most shell
      metacharacters from user and hostnames supplied via the command-line.

 -- Colin Watson <[email protected]>  Tue, 19 Dec 2023 14:51:56 +0000

That goes on for quite a few pages, listing all the changes back to when the package was first included in Debian with version 1.2 in 1999. Maintaining that manually would be a pain, so if there's a way to do that via GitHub actions, that would really be ideal. I'm just not sure what the options are for generating/appending text to the beginning of a file that way.

But, if that can be generated, then the relative path to it can be included in Cargo.toml and it'll be incorporated into the package.

@SteveLauC
Copy link
Member

I'll need to look into whether they accept packages that can't be built with available Debian packages. If they do, then there's actually one more change (that I know of, anyway) which would need to happen for it to be submitted and isn't currently addressed here. I didn't do this bit because I'm not quite sure what the best way would be to set it up (and, unless the package is being submitted to the Debian repo, probably doesn't matter to much).

What would need to be added is the changelog.

Thanks for showing me this! Topgrade indeed does not keep changelogs, we currently only keep breaking changes. At Nix, we do leave a changelog for the PRs that deserve it, we use towncrier to generate it on every release, but from the changelog of OpenSSH, the format needed by Debian packages is quite different from the one we use at Nix.

Anyway, having .deb files in the release assets as the first step looks pretty good, thanks for the PR!

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@SteveLauC SteveLauC merged commit 7b2623e into topgrade-rs:main Sep 4, 2024
11 checks passed
@SteveLauC SteveLauC mentioned this pull request Sep 4, 2024
@Kreeblah Kreeblah deleted the deb_packaging branch September 4, 2024 05:08
@gmkey
Copy link
Contributor

gmkey commented Mar 12, 2025

Hi @Kreeblah:
I just found this tool. Wondering if it was possible to create an apt repo that resides outside the Debian tree.
https://github.com/rpatterson/github-apt-repos

Having said that: it would probably not be the wisest approach. I know some Debian contributors. I'll see if they can help us cross the gap.

@Kreeblah
Copy link
Contributor Author

Hi @Kreeblah: I just found this tool. Wondering if it was possible to create an apt repo that resides outside the Debian tree. https://github.com/rpatterson/github-apt-repos

Having said that: it would probably not be the wisest approach. I know some Debian contributors. I'll see if they can help us cross the gap.

Hey @gmkey!

I'd honestly be hesitant to use Github as a package repo. Especially since it'd mean probably adding another dependency to the CI tools. @SteveLauC would obviously have final say, but . . . well, it feels kind of fragile and that it would add a fair bit of complexity (especially since it would add signing key management into the mix).

If it could be added to the Debian repo, that would be ideal, but I've been assuming at least the changelog situation would be a showstopper there (plus possibly the rust version issue). But, maybe it wouldn't be? I'd be curious to find out what the contributors you know say.

In the meantime, I have an Ansible role for installing Topgrade on macOS, FreeBSD, and Debian-based Linux systems that, for Debian-based systems, compares the installed version to whatever the latest version is and updates if required. It's also not ideal, but at least works for me for the time being.

@SteveLauC
Copy link
Member

@SteveLauC would obviously have final say

On Topgrade's repo side, I am ok with any solutions that won't make the maintenance harder. Out of Topgrade's repo, I have no control there, feel free to do whatever helps the most:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants