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

Include Debian package in release artifacts #172

Merged
merged 7 commits into from
Sep 6, 2021
Merged

Conversation

ducaale
Copy link
Owner

@ducaale ducaale commented Sep 5, 2021

Resolves #171

@blyxxyz blyxxyz self-requested a review September 5, 2021 16:11
Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Lintian is a linter for Debian packages. Here's what it has to say:

$ lintian xh_0.12.0_amd64.deb
E: xh: no-changelog usr/share/doc/xh/changelog.gz (native package)
W: xh: copyright-without-copyright-notice
W: xh: shared-library-lacks-prerequisites usr/bin/xh
W: xh: unknown-field xh_0.12.0_amd64.deb Vcs-Browser
W: xh: unknown-field xh_0.12.0_amd64.deb Vcs-Git

E: xh: no-changelog usr/share/doc/xh/changelog.gz (native package)
W: xh: copyright-without-copyright-notice

I think these are covered by the review comments.

W: xh: shared-library-lacks-prerequisites usr/bin/xh

I can't make sense of this. I think it's complaining that the binary is dynamically linked while the package doesn't depend on libc. But:

$ file usr/bin/xh
usr/bin/xh: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, BuildID[sha1]=cc1900e452326a1d7aeb1d39cd7bc6e99dd6036b, stripped
$ ldd usr/bin/xh
	statically linked

The binary works in an Alpine docker container so it's probably fine.

W: xh: unknown-field xh_0.12.0_amd64.deb Vcs-Browser
W: xh: unknown-field xh_0.12.0_amd64.deb Vcs-Git

I don't think these matter.

debian/postinst Outdated
#!/bin/sh

set -e
ln -sf /usr/bin/xh /usr/bin/xhs
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not an ideal way to make the symlink, it won't be tracked by dpkg.

I peeked inside Debian's zstd package. It packages its aliases as ordinary relative symlinks, like zstdcat -> zstd.

You have to take some care to make the symlink come out like that. If you do it right then ls -l will give output like the above and the symlink will work as long as the file it points to is in the same directory no matter where that directory is.

I think this would work, though I haven't tried it and don't know if it's the best solution:

  • Create a symlink with ln -s xh path/to/xhs (best checked into source control, I think, to more easily build a deb manually)
  • Add ["path/to/xhs", "usr/bin/", "777"], to the assets (not 100% sure about the permissions, but I do see lrwxrwxrwx for all the symlinks in my /usr/bin yet I can't modify them)
  • Enable cargo-deb's preserve-symlinks option

Cargo.toml Outdated
maintainer-scripts = "debian"
assets = [
["target/release/xh", "usr/bin/", "755"],
["LICENSE", "usr/share/doc/xh/", "644"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be omitted and replaced by the license-file option. That way it should end up in the copyright file that packages typically use.

Cargo.toml Outdated
assets = [
["target/release/xh", "usr/bin/", "755"],
["LICENSE", "usr/share/doc/xh/", "644"],
["CHANGELOG.md", "usr/share/doc/xh/CHANGELOG", "644"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

The norm is for the changelog to go in a file called NEWS.gz (or changelog.gz, but that's deprecated). (The changelog option does not do this, it's for the packaging changelog (changelog.Debian.gz).)

I think the best option for now is to put it at usr/share/doc/xh/NEWS and file an issue/PR for cargo-deb to automatically compress it. It already has such handling for man pages.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Do you plan to open an issue for this in cargo-deb?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I opened mmstick/cargo-deb#193.

Until they are merged we can use cargo install --git https://github.com/blyxxyz/cargo-deb --branch xh-patches cargo-deb.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was wrong about NEWS.gz and changelog.gz being equivalent. The first is for release notes, the second is a more complete changelog.

Lintian would like changelog.gz to exist. It looks like it's sometimes generated from a git log. I'm not quite sure how to handle that, but I think it's ok to skip for now. It's something cargo-deb might be able to do automatically at some point if it detects a git repository.

(So this doesn't need further changes.)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I am a bit curious about the difference between NEWS.gz and changelog.gz. Do you know of any examples?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One example is coreutils.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Now I see the difference, thanks!

@ducaale
Copy link
Owner Author

ducaale commented Sep 6, 2021

I can't make sense of this. I think it's complaining that the binary is dynamically linked while the package doesn't depend on libc. But:

$ file usr/bin/xh
usr/bin/xh: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, >BuildID[sha1]=cc1900e452326a1d7aeb1d39cd7bc6e99dd6036b, stripped
$ ldd usr/bin/xh
  statically linked

Hmm, this is what I see when I use the same commands with the last x86_64-unknown-linux-musl binary that was released:

$ file /home/ducaale/.local/bin/xh
/home/ducaale/.local/bin/xh: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, stripped
$ ldd /home/ducaale/.local/bin/xh
        not a dynamic executable

assets/xh Outdated
@@ -0,0 +1 @@
This is a temp file to make cargo-deb happy while copying the xhs symlink.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've filed mmstick/cargo-deb#192 to avoid needing this.

@blyxxyz
Copy link
Collaborator

blyxxyz commented Sep 6, 2021

Lintian has a new complaint: W: xh: no-manual-page usr/bin/xhs

ln -s xh.1.gz assets/xhs.1.gz can solve it, if the filename already ends in .gz then it won't trigger cargo-deb's auto-compression.

We should also mention xhs in the man page itself but that can happen outside this PR, Lintian doesn't care.

Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

I think this is ready. It's very nice to have it, thank you!

It's missing shell completions for xhs, but that's not as simple as a symlink so that can come later.

@ducaale ducaale merged commit 526a55d into develop Sep 6, 2021
@ducaale ducaale deleted the debian-package branch September 6, 2021 21:16
@blyxxyz blyxxyz mentioned this pull request Sep 6, 2021
3 tasks
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.

None yet

2 participants