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.

Sign up for GitHub

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

Jump to bottom

glibc: 2.38-23 -> 2.38-27 #258972

Merged
merged 1 commit into from
Oct 4, 2023
Merged

glibc: 2.38-23 -> 2.38-27 #258972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified pkgs/development/libraries/glibc/2.38-master.patch.gz
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a binary checked into git?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's explained in common.nix:

      /* No tarballs for stable upstream branch, only https://sourceware.org/git/glibc.git and using git would complicate bootstrapping.
          $ git fetch --all -p && git checkout origin/release/2.38/master && git describe
          glibc-2.38-27-g750a45a783
          $ git show --minimal --reverse glibc-2.38.. | gzip -9n --rsyncable - > 2.38-master.patch.gz

         To compare the archive contents zdiff can be used.
          $ zdiff -u 2.38-master.patch.gz ../nixpkgs/pkgs/development/libraries/glibc/2.38-master.patch.gz

Copy link
Member

Choose a reason for hiding this comment

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

This explains the need for the patch but not why it's gzipped?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it's already 3KB without being gzipped, and people are careful about not increasing the size of the nixpkgs repo.

Copy link
Member

Choose a reason for hiding this comment

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

Don't people always download the Nixpkgs repo compressed in some way, though? If you do a git clone, you get compressed packs. If you download a channel, you get a tar.xz. If you download from GitHub, you get a zip or a tar.gz or whatever. I'd expect this file being a .gz to make the compressed size of Nixpkgs as a whole slightly worse, because now it's opaque to the compressor and can't be deduplicated with other similar files.

Copy link
Member

Choose a reason for hiding this comment

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

We sometimes backport security fixes and tests that validate they work. In some cases the test artifacts are in fact binary (think image formats, asn1, ...), but we'd still like to have them, to ensure the patch works, and does not regress.

This is especially relevant, since fetchpatch drops binary hunks, and even then we sometimes need to put a rebased patch into the tree.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds different though — the patch itself would still be a text file, right?

Copy link
Member

Choose a reason for hiding this comment

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

Those artifacts should then either be generated from text-based formats or be fetched from somewhere else.

Binaries in Git are incredibly inefficient in both size and, more importantly, performance and Nixpkgs can already be a challenge in those regards.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm a little late to the party 😅

I just skimmed through old PRs and it seems as if we started at around 2.31 or 2.32 with that when we needed to apply a bunch of patches for several issues and decided to just follow the release branch.
I guess it sounds reasonable to switch to a plaintext patchset especially given @alyssais' explanation on the expected influence on nixpkgs tarballs.

cc @TredwellGit @vcunat in case they have anything to add.

Copy link
Member

Choose a reason for hiding this comment

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

fwiw #287594 contains a patch rather than a gz file.

Binary file not shown.
4 changes: 2 additions & 2 deletions pkgs/development/libraries/glibc/common.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@

let
version = "2.38";
patchSuffix = "-23";
patchSuffix = "-27";
sha256 = "sha256-+4KZiZiyspllRnvBtp0VLpwwfSzzAcnq+0VVt3DvP9I=";
in

Expand All @@ -60,7 +60,7 @@ stdenv.mkDerivation ({
[
/* No tarballs for stable upstream branch, only https://sourceware.org/git/glibc.git and using git would complicate bootstrapping.
$ git fetch --all -p && git checkout origin/release/2.38/master && git describe
glibc-2.38-23-g0e1ef6779a
glibc-2.38-27-g750a45a783
Ma27 marked this conversation as resolved.
Show resolved Hide resolved
$ git show --minimal --reverse glibc-2.38.. | gzip -9n --rsyncable - > 2.38-master.patch.gz

To compare the archive contents zdiff can be used.
Expand Down