Make default.nix configurable #3364

dbaynard · 2024-03-31T16:53:40Z

The default.nix defines various values that consumers may which to override, such as the nixpkgs version. These variables can be declared instead in the arguments to the top level function, with the current values set as defaults.

Part of #3026

wolfgangwalther · 2024-03-31T17:22:55Z

default.nix

+, applyPatches ? (
+    # If nixpkgs is a flake
+    nixpkgs.legacyPackages.${system}
+      or
+      # If nixpkgs is a derivation
+      import nixpkgs
+      { inherit system; }
+  ).applyPatches


Regarding applyPatches.. this is supposed to go away very soon. As soon as I can update nixpkgs to the latest master, we won't need it anymore. Evaluating nixpkgs twice really causes a few issues with caching in CI, too.

I think we are currently only blocked by NixOS/nixpkgs#299136.

Ah, I hadn't realized that would cause CI issues ~~— I'd guess it's because the patch source pulls in the entire repo (edit: I'm sure I've seen this in the past, but now I think about it, I am sceptical)?~~

In any case, it's plausible there will be future cases where nixpkgs needs to be patched, even if temporarily. This at least avoids one extra import (if using flakes).

Ah, I hadn't realized that would cause CI issues — I'd guess it's because the patch source pulls in the entire repo (edit: I'm sure I've seen this in the past, but now I think about it, I am sceptical)?

Here, it causes a problem with some magic I have added here:

postgrest/.github/workflows/build.yaml

Lines 73 to 82 in 06ff56d

- name: Build everything

run: |

# The --dry-run will give us a list of derivations to download from cachix and

# derivations to build. We only take those that would have to be built and then build

# those explicitly. This has the advantage that pure verification will not include

# a download anymore, making it much faster. If something needs to be built, only

# the dependencies required to do so will be downloaded, but not everything.

nix-build --dry-run 2>&1 \

| gsed -e '1,/derivations will be built:$/d' -e '/paths will be fetched/Q' \

| xargs nix-build

This is to avoid downloading stuff from cachix which doesn't need to be rebuild. But the patched nixpkgs is not cached in cachix (actually not sure why) and thus causes full downloads of all packages depending on it.

In any case, it's plausible there will be future cases where nixpkgs needs to be patched, even if temporarily.

I guess the proper approach here would be to temporarily point nixpkgs at e.g. my fork where the patch is applied already. Not sure how that would interact with flakes, though.

I guess the proper approach here would be to temporarily point nixpkgs at e.g. my fork where the patch is applied already. Not sure how that would interact with flakes, though.

With flakes it's straightforward. Actually, this could be done now — keep a postgrest fork of nixpkgs and refer to that in nixpkgsVersions etc. Then, with a flake, one would set nixpkgs to the upstream version or the fork. Stepping back further, a better approach would be for the Haskell-wrapper to be first class so one could override it.

This is to avoid downloading stuff from cachix which doesn't need to be rebuild.

This is a bit of an annoyance. I think I've used nix-build-uncached in the past but it's been a while since I've needed to reach for this sort of thing.

But the patched nixpkgs is not cached in cachix (actually not sure why) and thus causes full downloads of all packages depending on it.

I think this is one of those "quit thinking and look" cases… that said, it wouldn't surprise me if the derivation for the patched nixpkgs is depending on this repo source. If so, this PR might fix that, as I removed the overlays from the import of nixpkgs that defines applyPatches.

This is a bit of an annoyance. I think I've used nix-build-uncached in the past but it's been a while since I've needed to reach for this sort of thing.

Ah, nix-build-uncached does exactly the same thing I did here, just a bit more sophisticated. Seems like I re-invented the wheel. Thanks for pointing me that way, will try nix-fast-build --skip-cached, too.

dbaynard · 2024-03-31T17:22:57Z

@wolfgangwalther I would need to confirm that the change to applyPatches (specifically, the removal of overlays from the import) does reduce the instantations of nixpkgs in the store. It certainly does no harm, though it wouldn't be worth a PR on its own.

It's very annoying that patching nixpkgs requires importing from a source derivation. Hopefully fixes to NixOS/nixpkgs#286285 (and patching nixpkgs in flakes) will be forthcoming.

wolfgangwalther · 2024-03-31T17:24:20Z

Hopefully fixes to NixOS/nixpkgs#286285 (and patching nixpkgs in flakes) will be forthcoming.

The fix relevant for us was already merged to master, we're just blocked from updating to it at the moment (see other comment).

dbaynard · 2024-03-31T17:28:21Z

I've marked as ready for review — I guess you can drop the second commit, or wait until upstream is fixed.

wolfgangwalther · 2024-03-31T17:39:51Z

or wait until upstream is fixed.

I just gave an update in the blocking PR upstream. Will wait a bit to see how that turns out.

I like the approach you took here.

dbaynard · 2024-03-31T17:40:52Z

A nice benefit of this is it makes it straightforward to test with different versions of nixpkgs and GHC.

wolfgangwalther · 2024-03-31T17:45:54Z

A nice benefit of this is it makes it straightforward to test with different versions of nixpkgs and GHC.

Yes. I imagine a future, where cross-compiling will also work nicely... and a simple github actions matrix job will just build for different platforms and GHC versions, all via nix. ❤️

dbaynard · 2024-03-31T19:14:46Z

I replied in the thread but this should be top level: the current dependency on overlays in nixpkgs-patched might be what is busting the cachix cache. This PR removes that dependency.

wolfgangwalther · 2024-04-20T15:07:38Z

@dbaynard #3365 is merged, you can rebase.

Moving values defined with `let` to the function arguments (with defaults) means other consumers of `default.nix` can customize these values. One example is a `flake.nix`, which can then supply the `nixpkgs` input.

dbaynard · 2024-04-20T22:11:01Z

Congratulations on getting (many) of your upstream PRs merged. This is much cleaner, without the patching.

I've pushed a basic flake to https://github.com/fore-stun/postgrest/tree/flake-2 and you can run a build with the following.

nix build -L 'github:fore-stun/postgrest/flake-2'

Or, using the revision,

nix build -L 'github:fore-stun/postgrest/1a61b4d1c4ee0ec52468052719af38e832c43d74'

I've only tested on MacOS (and it builds).

wolfgangwalther · 2024-05-09T20:15:53Z

I already merged the commit here after rebase + slighty formatting change in 28aac08. Somehow couldn't push to this PR, even though the " Maintainers are allowed to edit this pull request. " setting is there. Might be, because this is coming from the fore-stun:main branch - that could make a difference.

I will leave this PR open for a moment, though, because there are still two comments above, that I would like to follow up on for myself. Thus I assigned myself here - nothing else to be done here.

dbaynard force-pushed the main branch from e01b4fa to df3afad Compare March 31, 2024 17:19

wolfgangwalther reviewed Mar 31, 2024

View reviewed changes

dbaynard marked this pull request as ready for review March 31, 2024 17:27

dbaynard mentioned this pull request Apr 17, 2024

Nixpkgs update #3365

Merged

dbaynard force-pushed the main branch from df3afad to 53c9df3 Compare April 20, 2024 21:56

nix: Make default.nix extensible

627fd92

Moving values defined with `let` to the function arguments (with defaults) means other consumers of `default.nix` can customize these values. One example is a `flake.nix`, which can then supply the `nixpkgs` input.

dbaynard force-pushed the main branch from 53c9df3 to 627fd92 Compare April 20, 2024 22:05

dbaynard requested a review from wolfgangwalther April 20, 2024 22:06

wolfgangwalther self-assigned this May 9, 2024

wolfgangwalther mentioned this pull request May 20, 2024

ci: Improve caching #3211

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make default.nix configurable #3364

Make default.nix configurable #3364

dbaynard commented Mar 31, 2024

wolfgangwalther Mar 31, 2024

dbaynard Mar 31, 2024 •

edited

Loading

wolfgangwalther Mar 31, 2024

dbaynard Mar 31, 2024

wolfgangwalther Mar 31, 2024

dbaynard commented Mar 31, 2024

wolfgangwalther commented Mar 31, 2024

dbaynard commented Mar 31, 2024

wolfgangwalther commented Mar 31, 2024

dbaynard commented Mar 31, 2024

wolfgangwalther commented Mar 31, 2024

dbaynard commented Mar 31, 2024

wolfgangwalther commented Apr 20, 2024

dbaynard commented Apr 20, 2024 •

edited

Loading

wolfgangwalther commented May 9, 2024

	- name: Build everything
	run: \|
	# The --dry-run will give us a list of derivations to download from cachix and
	# derivations to build. We only take those that would have to be built and then build
	# those explicitly. This has the advantage that pure verification will not include
	# a download anymore, making it much faster. If something needs to be built, only
	# the dependencies required to do so will be downloaded, but not everything.
	nix-build --dry-run 2>&1 \
	\| gsed -e '1,/derivations will be built:$/d' -e '/paths will be fetched/Q' \
	\| xargs nix-build

Make default.nix configurable #3364

Are you sure you want to change the base?

Make default.nix configurable #3364

Conversation

dbaynard commented Mar 31, 2024

wolfgangwalther Mar 31, 2024

Choose a reason for hiding this comment

dbaynard Mar 31, 2024 • edited Loading

Choose a reason for hiding this comment

wolfgangwalther Mar 31, 2024

Choose a reason for hiding this comment

dbaynard Mar 31, 2024

Choose a reason for hiding this comment

wolfgangwalther Mar 31, 2024

Choose a reason for hiding this comment

dbaynard commented Mar 31, 2024

wolfgangwalther commented Mar 31, 2024

dbaynard commented Mar 31, 2024

wolfgangwalther commented Mar 31, 2024

dbaynard commented Mar 31, 2024

wolfgangwalther commented Mar 31, 2024

dbaynard commented Mar 31, 2024

wolfgangwalther commented Apr 20, 2024

dbaynard commented Apr 20, 2024 • edited Loading

wolfgangwalther commented May 9, 2024

dbaynard Mar 31, 2024 •

edited

Loading

dbaynard commented Apr 20, 2024 •

edited

Loading