auxolotl/core
22
1
Fork 0
Code Issues Pull requests 4 Actions Packages Projects Releases Wiki Activity

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
Conversation 3 Commits 4 Files changed 2 +16 -4
bbjubjub2494 commented 2024-05-14 09:49:45 +00:00 (Migrated from github.com)
Copy link

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)
Copy link

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

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)
Copy link

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)
Copy link

Removing the fix hash commit to avoid conflicts

Removing the fix hash commit to avoid conflicts
👍 1
Sorixelle (Migrated from github.com) reviewed 2024-05-19 08:11:11 +00:00
pkgs/build-support/fetchgithub/default.nix
Sorixelle (Migrated from github.com) commented 2024-05-19 08:11:11 +00:00
Copy link

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
pkgs/build-support/fetchgithub/default.nix
Sorixelle (Migrated from github.com) commented 2024-05-19 08:12:06 +00:00
Copy link

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
pkgs/build-support/fetchgithub/default.nix
Sorixelle (Migrated from github.com) commented 2024-05-19 08:18:54 +00:00
Copy link

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
pkgs/build-support/fetchgithub/default.nix
bbjubjub2494 (Migrated from github.com) commented 2024-05-19 08:24:50 +00:00
Copy link

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
pkgs/build-support/fetchgithub/default.nix
bbjubjub2494 (Migrated from github.com) commented 2024-05-19 08:41:34 +00:00
Copy link

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
pkgs/build-support/fetchgithub/default.nix
@ -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
Copy link

/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
pkgs/build-support/fetchgithub/default.nix
Sorixelle (Migrated from github.com) commented 2024-05-20 05:45:55 +00:00
Copy link

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
pkgs/build-support/fetchgithub/default.nix
Sorixelle (Migrated from github.com) commented 2024-05-20 05:47:07 +00:00
Copy link

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.
Reviewers
No reviewers
Sorixelle
Labels
Clear labels
  
Compat/Breaking

Breaking change that won't be backward compatible

  
Kind/Bug

Something is not working

  
Kind/Documentation

Documentation changes

  
Kind/Enhancement

Improve existing functionality

  
Kind/Feature

New functionality

  
Kind/Security

This is security issue

  
Kind/Testing

Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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.
Delete branch "fetchgithub-refetch"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?