Skip to content

Comments

Refactor setup for generated nix files#1223

Merged
mergify[bot] merged 2 commits intomasterfrom
joachim/nix/generated
Feb 20, 2020
Merged

Refactor setup for generated nix files#1223
mergify[bot] merged 2 commits intomasterfrom
joachim/nix/generated

Conversation

@nomeata
Copy link
Contributor

@nomeata nomeata commented Feb 20, 2020

the reiterates on #1220. The motivation for the refactoring was that the
generated files were not using gitSource, so I would get cache misses
and dirty files in the repo. Fixing this was easier if all generated nix
files are in one place, so I did the following changes:

  • All generated nix files are in nix/generated. This de-pollutes the
    normal working area with generated files, and keeps them out from
    sources (more precise derivations).
  • The local generated files now use gitSource.
  • The generator is now nix/generate.nix.
  • I moved complex logic from nix/default.nix to nix/generate.nix,
    this means everything is in one place, instead of spread over several
    nix files.
  • There is now only a single check to check if the generated files are
    upto date, and a single command to update the files.
  • This check is no longer marked allowSubstitutes = false. The check
    isn’t cheap (it requires cabal2nix) and we want the check to happen
    on builders, not hydra itself, so that the cache has cabal2nix.
  • The check is now included in all-systems-go.

the reiterates on #1220. The motivation for the refactoring was that the
generated files were not using `gitSource`, so I would get cache misses
and dirty files in the repo. Fixing this was easier if all generated nix
files are in one place, so I did the following changes:

 * All generated nix files are in `nix/generated`. This de-pollutes the
   normal working area with generated files, and keeps them out from
   sources (more precise derivations).
 * The local generated files now use `gitSource`.
 * The generator is now `nix/generate.nix`.
 * I moved complex logic from `nix/default.nix` to `nix/generate.nix`,
   this means everything is in one place, instead of spread over several
   nix files.
 * There is now only a single check to check if the generated files are
   upto date, and a single command to update the files.
 * This check is no longer marked `allowSubstitutes = false`. The check
    isn’t cheap (it requires `cabal2nix`) and we want the check to happen
    on builders, not hydra itself, so that the cache has `cabal2nix`.
 * The check is now included in `all-systems-go`.
@nomeata nomeata requested a review from basvandijk February 20, 2020 00:07
@nomeata nomeata marked this pull request as ready for review February 20, 2020 00:26
Copy link
Contributor

@pikajude pikajude left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@nomeata nomeata added the automerge-squash When ready, merge (using squash) label Feb 20, 2020
@mergify mergify bot merged commit 290534b into master Feb 20, 2020
@mergify mergify bot deleted the joachim/nix/generated branch February 20, 2020 23:33
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Feb 20, 2020
winter = check ./winter;
};
check-generated = nixpkgs.runCommandNoCC "check-generated" {
nativeBuildInputs = [ nixpkgs.diffutils ];
Copy link
Contributor

@basvandijk basvandijk Feb 21, 2020

Choose a reason for hiding this comment

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

For inexpensive builds like this I would use:

        preferLocalBuild = true;
        allowSubstitutes = false;

To let CI build this locally instead of shipping inputs and outputs to and from remote builders / cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But is it inexpensive? It still depends on cabal2nix, so you’d have to build that on hydra, rather than the builders… Maybe it is not a concern.

The other reason is that we want cabal2nix etc. to be available on the cache, and this is the derivation that uses it, and the cache is only fed by the builders, isn’t it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants