[staging] fetchFromGitHub: name source with content intristic to the value #153386
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @infinisil who has lead RFCs trying to get around this cc @domenkozar who had concerns around flake usage cc @edolstra to make sure I'm not dumb :) |
|
I believe ofborg timed out. As this is essentially a stdenv rebuild |
|
@GrahamcOfBorg eval |
jonringer
requested review from
aanderse,
cdepillabout,
etu,
expipiplus1,
FRidh,
globin,
Ma27,
maralorn,
matthewbauer,
sternenseemann,
talyz and
zowoq
as code owners
github-actions
bot
added
6.topic: haskell
6.topic: python
6.topic: rust
8.has: documentation
jonringer
force-pushed
the
avoid-fetchfromgithub-footgun
branch
from
21dc904 to
576e26b
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/merge-fast-and-break-things/16957/10 |
jonringer
requested review from
infinisil,
mmahut and
RaghavSood
as code owners
github-actions
bot
added
the
6.topic: nixos
jonringer
force-pushed
the
avoid-fetchfromgithub-footgun
branch
from
a8f43bb to
83660f9
Compare
mweinelt
reviewed
Jan 31, 2022
| @@ -287,7 +287,7 @@ in { | |||
| # Components that require access to serial devices (/dev/tty*) | |||
| # List generated from home-assistant documentation: | |||
| # git clone https://github.com/home-assistant/home-assistant.io/ | |||
| # cd source/_integrations | |||
| # cd ${src.name}/_integrations | |||
There was a problem hiding this comment.
I'm going to try and factor out all of the package updates to separate PRs, eventually this should just be the fetcher change and sha fixes
There was a problem hiding this comment.
I guess I could mark this as draft until then
There was a problem hiding this comment.
This is editing a comment that isn't fetcher related at all.
There was a problem hiding this comment.
correct, but eventually I'll fix it when pruning the commits :)
No, we shouldn't do it even temporarily (since temporary things have a tendency to become permanent). Like I said before, I don't want to go back to every fetcher in the Nix ecosystem producing inconsistent names. And it shouldn't be the case that changing the name of a repo or switching from a tag to a revision triggers a rebuild. |
I agree that this isn't ideal. However, there's not a single instance (that I could find) of I guess I'm optimizing for the human experience, and trying to avoid stale FODs. While retaining "source" as the derivation name is optimising for the nix experience. I think it makes sense for the exception be: "To use alongside builtins.fetch*, please pass With the changes in this PR, the default is: " |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/how-to-reference-src-directory-in-nix/18956/4 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/why-is-the-r-ryantm-bot-using-refs-tags-in-the-version/20519/7 |
infinisil
added
the
significant
|
Over a year ago, @Ericson2314 suggested waiting up to a year and then revisiting this. Can we revisit this? I would love to lose both the footgun of forgetting to update a hash and the confusion of looking at a list of paths named |
|
bump, I still think this is a major QoL win |
|
Building towards element-desktop this is what I see. 🤡 |
wegank
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
github-actions
bot
added
6.topic: Lumina DE
wegank
added
the
2.status: merge conflict
ShamrockLee
reviewed
Apr 3, 2024
| patchPhase = ''sed "s@ldconfig.*@@" -i source/Makefile''; | ||
| patchPhase = ''sed "s@ldconfig.*@@" -i ${src.name}/Makefile''; |
There was a problem hiding this comment.
The default name of the FOD produced by fetchurl is the basename of the URL, with file name extension included. In this case, it is currently "aeolus-0.10.4.tar.bz2", and the sensible value is "aeolus-0.10.4".
In my opinion, commands in phases should respect "$sourceRoot", so that overriding with src from a different fetcher is possible. I'll put the discussion under the RFC PR.
|
@oxij's recent work
... #49862 was born in 2018. It will go to school this year.
To be a bit more serious, though, yes, this appears to want to do exactly the same thing my PR does, motivation here is part of my motivation there, and I got exactly the same pushback on it.
Though, I did recently propose a solution to make everyone happy by adding a `nixpkgs.config` option so Hydra can have its `*-source` and developers can have all those sources named prettily in `/nix/store` and force rebuilds on revision change, exactly as here, yes.
|
13 tasks
wegank
added
the
2.status: stale
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now an RFC
NixOS/rfcs#171
Pre-RFC text:
Motivation for this change
DRAFT until package failures around src name are factored out
Just giving the derivation a name of "source" by default causes
a lot of pain when someone may update the rev, but fail to
compute the new sha256 or invalidate the existing sha256. A stale
sha256 will still be "valid" in the eyes of nix, as only the way
in which the sources are fetched has changed, but the previous
.drv and sha256 relationship is still preserved.
This change forces the FODs to be recomputed as the related .drv
has changed.
This will not require all of the FOD sha256s to be recomputed, as
the contents will not have changed. However, it will be a mass
rebuild as it will create a new DAG for packages which didn't
already pass a "name" value to fetchFromGitHub.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes