fetchFromGitHub: force re-fetch when rev changes #11

Merged
bbjubjub2494 merged 4 commits from fetchgithub-refetch into main 2024-05-21 05:45:52 +00:00
bbjubjub2494 commented 2024-05-14 09:49:45 +00:00 (Migrated from github.com)

Since we have yet to hit the ground running, we have a golden opportunity to fix some of those long-standing issues in upstream that don't get fixed to avoid disturbing the whole project. Specifically, this is the fact that fetchFromGitHub currently outputs a FOD with a very generic name. As a result, if a maintainer changes a version number but forgets about the output hash, it may not get caught. Error messages are also less useful. If we can fix that we will have much better DX moving forward.

To illustrate: I tried to rebuild nixdoc to see if it was working and I immediately caught a stale hash.

Based on https://github.com/NixOS/nixpkgs/pull/294068

Since we have yet to hit the ground running, we have a golden opportunity to fix some of those long-standing issues in upstream that don't get fixed to avoid disturbing the whole project. Specifically, this is the fact that `fetchFromGitHub` currently outputs a FOD with a very generic name. As a result, if a maintainer changes a version number but forgets about the output hash, it may not get caught. Error messages are also less useful. If we can fix that we will have much better DX moving forward. To illustrate: I tried to rebuild `nixdoc` to see if it was working and I immediately caught a stale hash. Based on https://github.com/NixOS/nixpkgs/pull/294068
isabelroses commented 2024-05-14 12:38:19 +00:00 (Migrated from github.com)
Is this https://github.com/NixOS/nixpkgs/commit/c3255fe8ec32#commitcomment-25289921, not a concern which lead the original PR maker to create https://github.com/NixOS/nixpkgs/pull/294329
bbjubjub2494 commented 2024-05-14 13:16:48 +00:00 (Migrated from github.com)

Is this NixOS/nixpkgs@c3255fe8ec32#commitcomment-25289921, not a concern which lead the original PR maker to create NixOS/nixpkgs#294329

This is true and those two commits are already included in the first PR as well as the present PR.

This change does not by itself invalidate all hashes. I checked the python3Packages.build hash and it was really stale to begin with.

> Is this [NixOS/nixpkgs@c3255fe8ec32#commitcomment-25289921](https://github.com/NixOS/nixpkgs/commit/c3255fe8ec32#commitcomment-25289921), not a concern which lead the original PR maker to create [NixOS/nixpkgs#294329](https://github.com/NixOS/nixpkgs/pull/294329) This is true and those two commits are already included in the first PR as well as the present PR. This change does *not* by itself invalidate all hashes. I checked the `python3Packages.build` hash and it was really stale to begin with.
bbjubjub2494 commented 2024-05-18 08:31:18 +00:00 (Migrated from github.com)

Removing the fix hash commit to avoid conflicts

Removing the fix hash commit to avoid conflicts
Sorixelle (Migrated from github.com) reviewed 2024-05-19 08:11:11 +00:00
Sorixelle (Migrated from github.com) commented 2024-05-19 08:11:11 +00:00

TBH, I'd also change source to something like github-repo, just to make it clearer in the name that the derivation comes from a GitHub checkout.

TBH, I'd also change `source` to something like `github-repo`, just to make it clearer in the name that the derivation comes from a GitHub checkout.
Sorixelle (Migrated from github.com) reviewed 2024-05-19 08:12:06 +00:00
Sorixelle (Migrated from github.com) commented 2024-05-19 08:12:06 +00:00

nit:

, name ? null # Override with null to use the default value
nit: ```suggestion , name ? null # Override with null to use the default value ```
Sorixelle (Migrated from github.com) reviewed 2024-05-19 08:18:54 +00:00
Sorixelle (Migrated from github.com) commented 2024-05-19 08:18:54 +00:00

Any specific reason for overrideAttrs over //?

Any specific reason for `overrideAttrs` over `//`?
bbjubjub2494 (Migrated from github.com) reviewed 2024-05-19 08:24:50 +00:00
bbjubjub2494 (Migrated from github.com) commented 2024-05-19 08:24:50 +00:00

Here's a slightly different proposal: append the github base url as provenance like so: /nix/store/wzvnh3p7gv1qlqhb3iif878bfffzrvaz-source-pypa-build-refs-tags-1.1.1-github.com.drv. That's a good self-description, leaves the source that people might be used to, and generalizes unambiguously to other fetchFroms. We can also put it before the owner.

Here's a slightly different proposal: append the github base url as provenance like so: `/nix/store/wzvnh3p7gv1qlqhb3iif878bfffzrvaz-source-pypa-build-refs-tags-1.1.1-github.com.drv`. That's a good self-description, leaves the `source` that people might be used to, and generalizes unambiguously to other `fetchFrom`s. We can also put it before the `owner`.
bbjubjub2494 (Migrated from github.com) reviewed 2024-05-19 08:41:34 +00:00
bbjubjub2494 (Migrated from github.com) commented 2024-05-19 08:41:34 +00:00

It's not super clear to me. The original author explains it as:

Enhance overriding for result FOD of fetchFromGitHub with .overrideAttrs and passthru.

  • Update the meta attribute of the result package by .overrideAttrs the one returned by the base fetcher.
  • Attach owner, repo and rev attributes via passthru instead of directly updating the attribute set via //, preserving these attributes across .overrideAttrs.

https://github.com/NixOS/nixpkgs/pull/294329

It's true that if someone re-overrides after the //, the meta will be lost. However, since fetchFromGitHub overrides overrideAttrs anyway (with makeOverridable) I don't think this can happen in practice. Better safe than sorry?

It's not super clear to me. The original author explains it as: > Enhance overriding for result FOD of fetchFromGitHub with <pkg>.overrideAttrs and passthru. > - Update the meta attribute of the result package by <pkgs>.overrideAttrs the one returned by the base fetcher. > - Attach owner, repo and rev attributes via passthru instead of directly updating the attribute set via //, preserving these attributes across <pkg>.overrideAttrs. https://github.com/NixOS/nixpkgs/pull/294329 It's true that if someone re-overrides after the `//`, the meta will be lost. However, since `fetchFromGitHub` overrides `overrideAttrs` anyway (with `makeOverridable`) I don't think this can happen in practice. Better safe than sorry?
bbjubjub2494 (Migrated from github.com) reviewed 2024-05-19 08:47:48 +00:00
@ -5,1 +4,4 @@
{ owner, repo, rev
, name ? null # Override with null to use the default value
, pname ? "source-${githubBase}-${owner}-${repo}"
, fetchSubmodules ? false, leaveDotGit ? null
bbjubjub2494 (Migrated from github.com) commented 2024-05-19 08:47:48 +00:00

/nix/store/f291banfgj2vjz1qsvmpm09awn2gl4pj-source-github.com-pypa-build-refs-tags-1.1.1.drv wdyt @Sorixelle ?

`/nix/store/f291banfgj2vjz1qsvmpm09awn2gl4pj-source-github.com-pypa-build-refs-tags-1.1.1.drv` wdyt @Sorixelle ?
Sorixelle (Migrated from github.com) reviewed 2024-05-20 05:45:56 +00:00
Sorixelle (Migrated from github.com) commented 2024-05-20 05:45:55 +00:00

Only potential issue I see is creating some pretty long path names, but I don't see it being a massive issue. Seems fine to me.

Only potential issue I see is creating some pretty long path names, but I don't see it being a *massive* issue. Seems fine to me.
Sorixelle (Migrated from github.com) reviewed 2024-05-20 05:47:07 +00:00
Sorixelle (Migrated from github.com) commented 2024-05-20 05:47:07 +00:00

Mhmm. Sounds good to me.

Mhmm. Sounds good to me.
Sorixelle (Migrated from github.com) approved these changes 2024-05-20 05:48:39 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: auxolotl/core#11
No description provided.