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

gitignoreSource: ensure Nix store path determinism #11

Merged
merged 1 commit into from Aug 19, 2019
Merged

gitignoreSource: ensure Nix store path determinism #11

merged 1 commit into from Aug 19, 2019

Conversation

lukateras
Copy link
Contributor

This ensures that, when used in situations like gitignoreSource ./., both Hydra and local checkout will have the exact same Nix path, which is important for reproducibility and being able to leverage binary cache.

This ensures that, when used in situations like `gitignoreSource ./.`,                                          
both Hydra and local checkout will have the exact same Nix path.
@domenkozar domenkozar merged commit 6e75696 into hercules-ci:master Aug 19, 2019
@lukateras
Copy link
Contributor Author

lukateras commented Aug 19, 2019

Wow, thank you for the fast merge! :)

@yorickvP
Copy link

yorickvP commented Sep 2, 2019

I support having this in here. Actually, I think cleanSourceWith should do this. However,

  1. having 'source' as the path name leads to very unclear error messages.
  2. This PR breaks composability with other cleanSourceWith's.

My old workaround for this problem, which is composable with other cleanSource's:

let
  toConstantSource = name: src: if src ? _isLibCleanSourceWith then builtins.path {
    inherit name;
    inherit (src) filter;
    path = src.origSrc;
  } else builtins.path { inherit name; path = src; };
  constGitIgnore = name: path: toConstantSource name (gitignoreSource path);

@lukateras
Copy link
Contributor Author

lukateras commented Sep 2, 2019

  1. having 'source' as the path name leads to very unclear error messages.

For the record, source is pretty idiomatic. For example, see fetchzip and by extension fetchers built on top of it (like fetchFromGitHub): https://github.com/NixOS/nixpkgs/blob/f3282c8d1e0ce6ba5d9f6aeddcfad51d879c7a4a/pkgs/build-support/fetchzip/default.nix#L14

Which implies that majority of Nixpkgs stdenv derivations have src that uses name = "source". It seems that your concern is similar to that in NixOS/nixpkgs#49862.

  1. This PR breaks composability with other cleanSourceWith's.

True. And that was listed as a feature: https://github.com/hercules-ci/gitignore/blob/ec5dd0536a5e4c3a99c797b86180f7261197c124/README.md#features

However, it's still possible to compose with gitignoreFilter.

@lukateras
Copy link
Contributor Author

lukateras commented Sep 2, 2019

@yorickvP: Also, your workaround doesn't seem to work for me as of Nix 2.2.2. When I try to build a derivation that uses src = lib.cleanSource (constGitIgnore "source" ./.), I get:

string '/nix/store/rwlmxrxs27x7lplhgji74rbfjdxl3nla-source' cannot refer to other paths, at /nix/store/jc62lsfijc5qab87yrbi6aag67jd0lr8-source/lib/sources.nix:49:17

Seems to me that's because constGitIgnore in your example returns builtins.path without all of attributes that cleanSourceWith returns. I'd guess the idea is that it only composes to the right, with constGitIgnore always being the final filter.

@yorickvP
Copy link

yorickvP commented Sep 2, 2019

@yegortimoshenko yeah, it terminates composability, but can still be used as the end to a composed set of cleanSource's.

Regarding (1), having package names in the nix store path may be a mistake altogether.

roberth added a commit that referenced this pull request Sep 6, 2019
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