polkadot: fix commit hash fetch for version string

[andresilva]

Motivation for this change

The comment in the code describes the issue and solution.

Before:

❯ polkadot --version
polkadot 0.9.16-unknown-x86_64-linux-gnu

After:

❯ polkadot --version
polkadot 0.9.16-41ab002-x86_64-linux-gnu

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.05 Release Notes (or backporting 21.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[andresilva]

/marvin opt-in
/status needs_reviewer

[marvin-mk2]

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

[kevincox]

Do you know where this is set? I think it would be much cleaner and more maintainable to submit an upstream patch to read the version from an environment variable (if this isn't already available). This will be useful for multiple consumers.

[kevincox]

Likely here: https://github.com/paritytech/substrate/blob/22ed351d86b3b04ca1df82fb0036c6376bf1fea0/utils/build-script-utils/src/version.rs#L23. It should be simple to make this code just return early if SUBSTRATE_CLI_IMPL_VERSION is already set.

[andresilva]

Yes, my plan is to add support in upstream for reading the commit hash from an environment variable. I am one of the maintainers of the project so I don't think there will be any issues getting it through although it will still take some versions to get it released.

[andresilva]

It's set here btw https://github.com/paritytech/substrate/blob/master/utils/build-script-utils/src/version.rs#L23, not on polkadot itself but substrate which is a dependency.

[kevincox]

Sounds good. Would it be reasonable to file an issue in the upstream project (or in nixpkgs if that is not wanted) and link to it from this comment just to tie everything together and track the removal of the workaround?

[andresilva]

As discussed in the review a PR was submitted upstream to remove the need for the fakeGit hack: paritytech/substrate#10845. This will need to eventually reach polkadot (which depends on the upstream project), I will personally keep track of it and remove the hack when that's done (probably in v0.9.18).

[kevincox]

Result of nixpkgs-review pr 159279 run on x86_64-linux 1

1 package built:

• polkadot

[asymmetric]

@andresilva I actually don't think this is the best solution. There are reproducibility issues with leaving the .git directory in the store.

Another (IMHO clenaer option), is to have the fakeGit just return the version we have in the package.

[asymmetric]

@andresilva disregard the PR above, it was a brain-fart. I still think though that we shoudn't have merged this and instead waited for upstream to merge.

[andresilva]

We are not leaving .git in the store, that's why the whole fakeGit hack exists. We fetch .git, extract the commit hash and write it to a file, and then delete the .git folder.

[asymmetric]

Yes, but the .git directory is there at the time nix calculates the hash of the derivation, isn't it? That's where the lack of r13y can come from. Or at least that's how I understand the issue.

[andresilva]

Only the .git_commit file that we create will be part of the final derivation hash. AFAIK the reproducibility problem comes from the fact that .git folder doesn't have a stable representation, therefore if you build the derivation using different versions of git you could end up with different .git contents and consequently a different derivation hash. Since we are not including the .git folder in the derivation and only a file whose contents we control there should be no reproducibility issues. FWIW this isn't a solution that I came up with, other packages do similar things, for example:

nixpkgs/pkgs/development/tools/misc/coreboot-toolchain/default.nix

Lines 27 to 32 in 48d63e9

	leaveDotGit = true;
	postFetch = ''
	patchShebangs $out/util/crossgcc/buildgcc
	PATH=${lib.makeBinPath [ getopt ]}:$PATH $out/util/crossgcc/buildgcc -W > $out/.crossgcc_version
	rm -rf $out/.git
	'';

nixpkgs/pkgs/development/tools/misc/coreboot-toolchain/default.nix

Line 54 in 48d63e9

export CROSSGCC_VERSION=$(cat .crossgcc_version)