Simplify flake.nix by creating generic callPackage-able package.nix #9535

tfc · 2023-12-04T18:22:30Z

Motivation

With a parametrized package.nix file, it is now easier to understand and change the main nix derivation and its derived ones for testing, internal api docs etc.

thx a lot @Ericson2314

Context

This work item has been identified on the way towards meson.

Priorities

Add 👍 to pull requests you find important.

- Internal API docs once again work - configure skips checks for a bunch of things it doesn't need

The Perl bindings are not part of Nix, but a downstream package, so they don't belong in `package.nix`. They don't really belong as an attribute on `nix` either, but we can just leave that interface as is for now.

Ericson2314 · 2023-12-04T22:16:54Z

The package.nix is pretty crazy --- my fault. But I think that really is for the best because it faithfully is presenting the interface of the underlying build system, which is also pretty crazy!

I think this division of labor:

top level thin (currently flake) curates known-good configurations, presents simpler interface
package.nix presents the underlying thing in all its glory with little/no opinions / simplification / curating

is actually good, but it is certainly something to discuss.

edolstra · 2023-12-05T13:23:09Z

We actually intentionally moved everything into one file (204291f) because IMHO that's easier to maintain and read (since you don't have to look at half a dozen files to know what's going on). Splitting it up adds a lot of boilerplate code for passing arguments (like the first 37 lines of package.nix).

Parameterizing package.nix suffers from the general Nixpkgs problem that the options are undiscoverable.

Ericson2314 · 2023-12-05T15:56:22Z

@edolstra I had a feeling you might be skeptical :). There are a number of things to discuss, but I think these might be the important ones:

We actually intentionally moved everything into one file (204291f) because

To me at least, this does not feel like a a return to the pre-204291f way of doing things, because:

none of those files were really usable in isolation, but package.nix is usable in isolation, and indeed the "ultimate" most flexible thing that should be suitable for any downstream task
the old release-common.nix just like today's commonDeps in the flake.nix, and I object to both as scattering the definition of individual builds around in an unhelpful why --- why certain things but not others from commonDeps are used in the constituents is far from clear. I therefore think what we have with this PR is more consolidated and easier to read:
- package.nix is completely self contained and you can understand it without looking at the flake.nix
- the flake.nix is also readable without understanding exactly what package.nix is. The auto-passed arguments don't need to be thought about, and the manually overriden ones are pretty self-explanatory within the flake.nix

Splitting it up adds a lot of boilerplate code for passing arguments (like the first 37 lines of package.nix).

Parameterizing package.nix suffers from the general Nixpkgs problem that the options are undiscoverable.

These are indeed deficiencies with our current callPackage pattern, but they are deficiencies for which we don't yet have solutions, let alone standardized ones. (Subflakes are not yet a solution because Flake inputs do not yet work for these non-file-tree things.)

I think the right thing to do is stick to this established pattern in the ecosystem, and then try to think about things that improve it! I already thought in general use Nix's own flake.nix as a dogfooding exercise to identify issues with Flakes as we try to stabilize them --- I likewise think we should also use Nix's own package.nix as a dogfooding exercise to fix this problems too.

And while we're at it, I'd add the __forDefaults as another hack in package.nix that needs a proper solution --- not being able to augment the let-rec of the parameter defaults without adding a "fake" parameter" is annoying.

roberth

Finally working on the package expression won't involve looking for parentheses half the time, while anxious about possibly having to reindent 300 lines of pure conflict.

I might now actually point to this flake as a possible way to do it, if, oddly enough, someone isn't convinced to use flake-parts.

Also, love the documented package parameters. Not formally discoverable just yet, but most definitely a step in the right direction!

Admittedly, this is not a thorough review. I expect ~1 mistake and the most effective way to find it, is to merge. (unfortunately git diff --color-moved --ignore-space-change wasn't helpful to me this time)

roberth · 2023-12-05T17:54:15Z

And while we're at it, I'd add the __forDefaults as another hack in package.nix that needs a proper solution --- not being able to augment the let-rec of the parameter defaults without adding a "fake" parameter" is annoying.

This could be addressed by the Nixpkgs architecture team, although it's just a tip of the iceberg when it comes to the situation with

package function arguments
mkDerivation arguments
derivation arguments
package attributes

which all compose in custom ways without any overarching design and plenty of bugs remaining because it's so ad hoc.
@DavHau has done great work on this kind of thing, but currently we're stuck on how to actually deliver such a thing without regressing eval performance to any significant degree in Nixpkgs.
My latest thoughts on the subject are around here if you're eager to know.
However, as you might guess, I don't have enough bandwidth to act on yet another such project.
Everything taken together, I would kindly suggest not to worry too much about this topic for now. Certainly not in the context of this PR, which is just an incremental improvement.

Ericson2314 · 2023-12-05T21:04:19Z

Thanks for the positive feedback!

I expect ~1 mistake and the most effective way to find it, is to merge.

If we want to do this, I suggest we try to get Hydra back up to 100% first, e.g. by merging #9518. Then we at have a proper "control" for getting back up to 100% if this introduces any minor issues.

infinisil · 2023-12-07T00:49:19Z

configure.ac

Why does this need to include non-Nix changes? If it's only Nix changes, you should be able to verify correctness by making sure you get the same derivations. It's probably also cleaner that way.

The configure script changes are because the configure script wrongly still checks for a bunch of would-be mandatory stuff when passing --disable-build.

I could PR that separately before the rest, but note that due to switching to things like lib.enableFeature I wasn't going for 100% preserved hashes.

nix-diff is your friend in that case. (EDIT: on derivation paths)

infinisil · 2023-12-07T00:54:39Z

flake.nix

+ # Experimental fileset library: https://github.com/NixOS/nixpkgs/pull/222981
+ # Not an "idiomatic" flake input because:
+ # - Propagation to dependent locks: https://github.com/NixOS/nix/issues/7730
+ # - Subflake would download redundant and huge parent flake
+ # - No git tree hash support: https://github.com/NixOS/nix/issues/6044
+ inherit (import (builtins.fetchTarball { url = "https://github.com/NixOS/nix/archive/1bdcd7fc8a6a40b2e805bad759b36e64e911036b.tar.gz"; sha256 = "sha256:14ljlpdsp4x7h1fkhbmc4bd3vsqnx8zdql4h3037wh09ad6a0893"; }))
+ fileset;
+


This might be a good time to switch to stable file set combinators, now in NixOS 23.11!

The one thing that changed is that lib.fileset.intersect is now lib.fileset.intersection.

Oh I just saw #9546, probably best to do this PR independently of the update for now.

Sure can do after that, or if you backport those to 23.05 I'll happily use them right away!

package.nix

lowdown.nix

edolstra · 2023-12-11T13:55:40Z

package.nix

+ doInstallCheck
+ ;
+
+ doBuild = !finalAttrs.dontBuild;


This combined with the subsequent dontBuild = !attrs.doBuild feels weird. One of those seems superfluous...

This is convenience so we are always using do* identifiers. They are not redundant because attrs vs finalAttrs.

Ericson2314 · 2023-12-11T14:43:32Z

Nix team meeting notes:

Big wide ranging discussion on how this touched on other issues

callPackage avoids the flakes system parameter problem
But a few awkward Nixpkgs conventions (long arguments, complicated overriding) that need to be fixed in Nixpkgs someday are also exposed

Decision: agree to do this, but it shouldn't be considered part of the public interface of the repo (the way flake.nix is) yet.

Using `+` is Nixpkgs standard ideom for this, and helps avoid needless rebuilds somewhat.

Ericson2314 · 2023-12-13T21:02:44Z

OK I think I fixed macOS, and also @thufschmitt as of https://hydra.nixos.org/eval/1802827 basically fixed master, so we can merge this and then fix anything that hydra might find as it happens.

Issue introduced in NixOS#9535

cole-h · 2023-12-18T20:31:51Z

(EDIT: I can reproduce in the default devshell as well, so it's not limited to native-clangStdenvPackages and not solved by the above "Fix clang devshell" PR)

This broke make install for me:

$ cat repro.sh
#!/usr/bin/env bash
nix develop .#native-clangStdenvPackages -c bash -euo pipefail -c '
autoreconf --install --force --verbose
./configure $configureFlags --prefix=$(pwd)/out
make -j$(nproc) install doc_generate=no
'

$ repo.sh
autoreconf: export WARNINGS=
autoreconf: Entering directory '.'
autoreconf: configure.ac: not using Gettext
autoreconf: running: aclocal --force
autoreconf: configure.ac: tracing
autoreconf: configure.ac: not using Libtool
autoreconf: configure.ac: not using Intltool
autoreconf: configure.ac: not using Gtkdoc
autoreconf: running: /nix/store/nhp2wyp7wgar861mqaysc8fcl77q8m7w-autoconf-2.71/bin/autoconf --force
configure.ac:1: warning: AC_INIT: not a literal: "m4_esyscmd(bash -c "echo -n $(cat ./.version)$VERSION_SUFFIX")"
autoreconf: running: /nix/store/nhp2wyp7wgar861mqaysc8fcl77q8m7w-autoconf-2.71/bin/autoheader --force
autoreconf: configure.ac: not using Automake
autoreconf: Leaving directory '.'
checking for a sed that does not truncate output... /nix/store/4mca20b13q88s6llkr8mc468rh9l9bmr-gnused-4.9/bin/sed
checking build system type... x86_64-pc-linux-gnu
checking host system type... x86_64-pc-linux-gnu
checking for the canonical Nix system name... x86_64-linux
checking for gcc... clang
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether the compiler supports GNU C... yes
checking whether clang accepts -g... yes
checking for clang option to enable C11 features... none needed
checking whether the compiler supports GNU C++... yes
checking whether clang++ accepts -g... yes
checking for clang++ option to enable C++11 features... none needed
checking how to run the C preprocessor... clang -E
checking for ar... ar
checking for special C compiler options needed for large files... no
checking for _FILE_OFFSET_BITS value needed for large files... no
checking for dirent.h that defines DIR... yes
checking for library containing opendir... none required
checking for struct dirent.d_type... yes
checking that GCC bug 80431 is fixed... yes
checking for pubsetbuf... yes
checking for statvfs... yes
checking for pipe2... yes
checking for lutimes... yes
checking whether it is possible to create a link to a symlink... yes
checking for stdio.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for strings.h... yes
checking for sys/stat.h... yes
checking for sys/types.h... yes
checking for unistd.h... yes
checking for locale... yes
checking for bash... /nix/store/0rwyq0j954a7143p0wzd4rhycny8i967-bash-5.2-p15/bin/bash
checking for flex... /nix/store/nfhymsyisbwmax4nrz7krmzch4y5sb9b-flex-2.6.4/bin/flex
checking for bison... /nix/store/8w3nvb3n3bb1p7zknlh9yabswxn5aafr-bison-3.8.2/bin/bison
checking for dot... no
checking for lsof... lsof
checking for jq... /nix/store/wgqafnfqzqa6p51bb32wdmsplgz62nnj-jq-1.6-bin/bin/jq
checking for boostlib >= 1.66 (106600) includes in "/nix/store/afz926ghcvzh6glhvlbxnd8yc4yqzaq6-boost-1.79.0/lib/include"... no
checking for boostlib >= 1.66 (106600)... yes
checking whether -latomic is needed... no
checking pkg-config is at least version 0.9.0... yes
checking for libcrypto >= 1.1.1... yes
checking for libarchive >= 3.1.2... yes
checking for sqlite3 >= 3.6.19... yes
checking for libcurl... yes
checking for libeditline... yes
checking for libsodium... yes
checking for libbrotlienc libbrotlidec... yes
checking for libcpuid... yes
checking for libseccomp... yes
./configure: line 7492: sys/xattr.h: No such file or directory
checking for aws/s3/S3Client.h... yes
checking for bdw-gc... yes
checking for gtest_main... yes
checking for rapidcheck/gtest.h... yes
checking for nlohmann_json >= 3.9... yes
checking for lowdown >= 0.9.0... yes
checking for libgit2... yes
checking for setresuid... yes
checking for setreuid... yes
checking for lchown... yes
checking for strsignal... yes
checking for posix_fallocate... yes
checking for sysconf... yes
checking whether sandbox-shell has the standalone feature... disabled
configure: creating ./config.status
config.status: creating config.h
  GEN    Makefile.config
  INST   /home/vin/workspace/vcs/nix/out/include/nix/config.h
  MKDIR  /etc/profile.d/
  MKDIR  /etc/init/
install: cannot change permissions of ‘/etc/profile.d/’: No such file or directory
make: *** [scripts/local.mk:8: /etc/profile.d/] Error 1
make: *** Waiting for unfinished jobs....
install: cannot change permissions of ‘/etc/init/’: No such file or directory
make: *** [misc/upstart/local.mk:3: /etc/init/] Error 1
  CXX    src/build-remote/build-remote.o
  CXX    src/nix-build/nix-build.o
  CXX    src/nix-channel/nix-channel.o
  CXX    src/nix-collect-garbage/nix-collect-garbage.o
  CXX    src/nix-env/user-env.o
  CXX    src/nix-copy-closure/nix-copy-closure.o
  CXX    src/nix-env/nix-env.o

specifically, eff9b12:

eff9b12bc296213c3ba824e90869bcafc4103e1c is the first bad commit
commit eff9b12bc296213c3ba824e90869bcafc4103e1c
Author: Jacek Galowicz <jacek@galowicz.de>
Date:   Fri Dec 1 11:25:22 2023 +0000

    Further changes

 binary-tarball.nix |   6 ++-
 coverage.nix       |  35 ++++++++++++++++
 flake.nix          | 117 +++++++++++++----------------------------------------
 package.nix        |  88 ++++++++++++++++++++--------------------
 4 files changed, 109 insertions(+), 137 deletions(-)
 create mode 100644 coverage.nix

cole-h · 2023-12-18T21:01:39Z

Fixed in #9633.

tfc and others added 24 commits November 30, 2023 22:48

First step

c982198

Run statix

c64190e

little refactoring

f55ee7c

Further changes

eff9b12

Fix cross builds

2c3749a

Fix coverage.nix

ca59832

Create internal-api-docs.nix

118fa96

Fix stuff

19d41fb

Add installing unit test flags

0ca49b0

WIP

ce598ba

WIP

3d47e02

Merge remote-tracking branch 'upstream/master' into package-nix

c9838bb

Fix incorrect flag name

c71d987

More fixes

7b51086

Fix underlying build system so --disable-build works better

c160c62

- Internal API docs once again work - configure skips checks for a bunch of things it doesn't need

Remove uneeded file

7a7ad7c

Move binary-tarball.nix to scripts dir

e275f0a

Expose boehmgc-nix in overlay

60fe4dd

Factor out the installer script

77003a4

Add documenting comments to package.nix

f586155

Don't expose file sets anymore

a5a45e6

Slap on perl-bindings in the caller

7e2b1cc

The Perl bindings are not part of Nix, but a downstream package, so they don't belong in `package.nix`. They don't really belong as an attribute on `nix` either, but we can just leave that interface as is for now.

Add another configure flag assertion

6e0656c

Clean up two comments

14c26d6

tfc requested a review from edolstra as a code owner December 4, 2023 18:22

Ericson2314 requested a review from roberth December 4, 2023 22:14

Ericson2314 requested a review from infinisil December 4, 2023 22:17

roberth approved these changes Dec 5, 2023

View reviewed changes

Ericson2314 mentioned this pull request Dec 6, 2023

Update to nixos-23.11 #9546

Merged

infinisil reviewed Dec 7, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into package-nix

2220a4a

tomberek added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Dec 11, 2023

edolstra reviewed Dec 11, 2023

View reviewed changes

Ericson2314 self-assigned this Dec 11, 2023

Ericson2314 mentioned this pull request Dec 11, 2023

Fix failing NixOS tests on master #9596

Closed

Ericson2314 added 7 commits December 11, 2023 15:26

Merge remote-tracking branch 'upstream/master' into package-nix

ff992f8

Merge remote-tracking branch 'upstream/master' into package-nix

d904f64

Restore comment

19573f1

Move lowdown.nix to misc/

f10f0f1

flake.nix: Delete uneeded attrs0 binding

bf5804d

Delete stray install_name_tool call

28f2f31

package.nix: Avoid ${..} for conditional strings

2d24875

Using `+` is Nixpkgs standard ideom for this, and helps avoid needless rebuilds somewhat.

roberth approved these changes Dec 13, 2023

View reviewed changes

Ericson2314 enabled auto-merge December 13, 2023 21:11

Ericson2314 merged commit e6515bd into NixOS:master Dec 13, 2023
8 checks passed

Ericson2314 mentioned this pull request Dec 14, 2023

Split --disable-tests, fix cross builds #9611

Merged

jlesquembre added a commit to jlesquembre/nix that referenced this pull request Dec 14, 2023

Fix clang devshell

8d39c0c

Issue introduced in NixOS#9535

jlesquembre mentioned this pull request Dec 14, 2023

Fix clang devshell #9613

Merged

cole-h mentioned this pull request Dec 18, 2023

package: don't set sysconfdir in devShells #9633

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify flake.nix by creating generic callPackage-able package.nix #9535

Simplify flake.nix by creating generic callPackage-able package.nix #9535

tfc commented Dec 4, 2023

Ericson2314 commented Dec 4, 2023

edolstra commented Dec 5, 2023

Ericson2314 commented Dec 5, 2023 •

edited

Loading

roberth left a comment

roberth commented Dec 5, 2023

Ericson2314 commented Dec 5, 2023

infinisil Dec 7, 2023

Ericson2314 Dec 7, 2023

roberth Dec 7, 2023 •

edited

Loading

infinisil Dec 7, 2023

infinisil Dec 7, 2023

Ericson2314 Dec 7, 2023

edolstra Dec 11, 2023

Ericson2314 Dec 11, 2023

Ericson2314 commented Dec 11, 2023

Ericson2314 commented Dec 13, 2023

cole-h commented Dec 18, 2023 •

edited

Loading

cole-h commented Dec 18, 2023

Simplify flake.nix by creating generic callPackage-able package.nix #9535

Simplify flake.nix by creating generic callPackage-able package.nix #9535

Conversation

tfc commented Dec 4, 2023

Motivation

Context

Priorities

Ericson2314 commented Dec 4, 2023

edolstra commented Dec 5, 2023

Ericson2314 commented Dec 5, 2023 • edited Loading

roberth left a comment

Choose a reason for hiding this comment

roberth commented Dec 5, 2023

Ericson2314 commented Dec 5, 2023

infinisil Dec 7, 2023

Choose a reason for hiding this comment

Ericson2314 Dec 7, 2023

Choose a reason for hiding this comment

roberth Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

infinisil Dec 7, 2023

Choose a reason for hiding this comment

infinisil Dec 7, 2023

Choose a reason for hiding this comment

Ericson2314 Dec 7, 2023

Choose a reason for hiding this comment

edolstra Dec 11, 2023

Choose a reason for hiding this comment

Ericson2314 Dec 11, 2023

Choose a reason for hiding this comment

Ericson2314 commented Dec 11, 2023

Ericson2314 commented Dec 13, 2023

cole-h commented Dec 18, 2023 • edited Loading

cole-h commented Dec 18, 2023

Ericson2314 commented Dec 5, 2023 •

edited

Loading

roberth Dec 7, 2023 •

edited

Loading

cole-h commented Dec 18, 2023 •

edited

Loading