Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make default.nix configurable #3364

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

dbaynard
Copy link
Contributor

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

default.nix Outdated
Comment on lines 15 to 22
, applyPatches ? (
# If nixpkgs is a flake
nixpkgs.legacyPackages.${system}
or
# If nixpkgs is a derivation
import nixpkgs
{ inherit system; }
).applyPatches
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dbaynard dbaynard Mar 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

- 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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
Copy link
Member

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 dbaynard marked this pull request as ready for review March 31, 2024 17:27
@dbaynard
Copy link
Contributor Author

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

@wolfgangwalther
Copy link
Member

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
Copy link
Contributor Author

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

@wolfgangwalther
Copy link
Member

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
Copy link
Contributor Author

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.

@dbaynard dbaynard mentioned this pull request Apr 17, 2024
@wolfgangwalther
Copy link
Member

@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
Copy link
Contributor Author

dbaynard commented Apr 20, 2024

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 wolfgangwalther self-assigned this May 9, 2024
@wolfgangwalther
Copy link
Member

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.

@wolfgangwalther wolfgangwalther mentioned this pull request May 20, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants