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

Remove unnecessary extra-source-files #672

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

sjakobi
Copy link
Member

@sjakobi sjakobi commented Jul 11, 2020

More files than necessary were included from the nix submodule.

This also adds some missing eval-compare test files. The testsuite
now always runs the same number of tests, no matter the
buildFromSdist setting.

This also sorts the files from the submodule, so they are easier to
navigate.

Context: #610

@sjakobi
Copy link
Member Author

sjakobi commented Jul 11, 2020

In local testing in nix-shell, this seems to execute the same number of tests as before: 278.

@sjakobi
Copy link
Member Author

sjakobi commented Jul 11, 2020

Judging by the test failures, this looks like a nice learning opportunity for me! ;)

@sjakobi
Copy link
Member Author

sjakobi commented Jul 14, 2020

For my own reference: I can reproduce the test failures locally with

nix-build --arg buildFromSdist true

@sjakobi
Copy link
Member Author

sjakobi commented Jul 14, 2020

There are different test failures:

The Eval comparison tests all fail with

error: getting status of '/tmp/nix-build-hnix-0.9.0.drv-0/hnix-0.9.0/data/nix/corepkgs': No such file or directory
FAIL
      Exception: readCreateProcess: nix-instantiate "--eval" "--strict" "tests/eval-compare/XYZ.nix" (exit 1): failed

hnix/tests/EvalTests.hs

Lines 438 to 446 in c6f9ab8

genEvalCompareTests = do
td <- D.listDirectory testDir
let unmaskedFiles = filter ((==".nix") . takeExtension) td
let files = unmaskedFiles \\ maskedFiles
return $ testGroup "Eval comparison tests" $ map (mkTestCase testDir) files
where
mkTestCase td f = testCase f $ assertEvalFileMatchesNix (td </> f)

This is the failing step:

nixVal <- nixEvalFile fp

@sjakobi
Copy link
Member Author

sjakobi commented Jul 14, 2020

At this stage the test failures appear to be fixed, but there's still a difference in the number of tests that are run with --arg buildFromSdist true and ... false:

+    builtins.appendContext.nix:                                      OK (0.02s)
+    builtins.eq-bottom-00.nix:                                       OK (0.03s)
+    builtins.fromJSON-01.nix:                                        OK (0.02s)
+    builtins.getContext.nix:                                         OK (0.06s)
+    builtins.lessThan-01.nix:                                        OK (0.06s)
+    builtins.mapAttrs-01.nix:                                        OK (0.03s)
+    builtins.pathExists.nix:                                         OK (0.01s)
+    builtins.replaceStrings-01.nix:                                  OK (0.05s)
+    builtins.toJSON.nix:                                             OK (0.03s)
+    current-system.nix:                                              OK (0.02s)
+    ellipsis.nix:                                                    OK (0.02s)
+    paths-01.nix:                                                    OK (0.02s)
+    placeholder.nix:                                                 OK (0.01s)

All of these are Eval comparison tests

@sjakobi
Copy link
Member Author

sjakobi commented Jul 14, 2020

This is the test failure when I remove data/nix/corepkgs/buildenv.nix from the extra-source-files:

      eval-okay-search-path:                                         FAIL
        Exception: While evaluating at data/nix/tests/lang/eval-okay-search-path.nix:1:1:
        While evaluating at data/nix/tests/lang/eval-okay-search-path.nix:2:1:
        While evaluating at data/nix/tests/lang/eval-okay-search-path.nix:4:1:
        While evaluating at data/nix/tests/lang/eval-okay-search-path.nix:4:8:
        While evaluating at data/nix/tests/lang/eval-okay-search-path.nix:4:19:
        file 'nix/buildenv.nix' was not found in the Nix search path (add it's using $NIX_PATH or -I)

@sjakobi
Copy link
Member Author

sjakobi commented Jul 14, 2020

data/nix/corepkgs/derivation.nix OTOH is also required by nix-instantiate for the Eval comparison tests and eval-okay-eq-derivations.

@sjakobi sjakobi force-pushed the sjakobi/extra-source-files branch from 810695f to 0f1257f Compare July 14, 2020 13:34
@sjakobi
Copy link
Member Author

sjakobi commented Jul 14, 2020

A test failure in statistics:

      Quantile is CDF inverse:                                                               FAIL
        *** Failed! Falsified (after 20 tests):
        improperGammaDistr 8.078186019792698 9.877105048646426
        Double01 0.47842987214924804
        Quantile      = 75.04977575136812
        Probability   = 0.47842987214924804
        Probability'  = 0.47842987214925126
        Rel. error    = 6.72961066780661e-15
        Abs. error    = 3.219646771412954e-15
        Expected err. = 52.978171024074584
        Distance      = 58
        Err/est       = 1.094790531248113
        Use --quickcheck-replay=248001 to reproduce.

https://travis-ci.org/github/haskell-nix/hnix/jobs/707980815


EDIT: Reported in haskell/statistics#169.

@sjakobi sjakobi force-pushed the sjakobi/extra-source-files branch from 0f1257f to 6ba1843 Compare July 23, 2020 00:23
More files than necessary were included from the nix submodule.

This also adds some missing eval-compare test files. The testsuite
now always runs the same number of tests, no matter the
`buildFromSdist` setting.

This also sorts the files from the submodule, so they are easier to
navigate.
@sjakobi sjakobi force-pushed the sjakobi/extra-source-files branch from 6ba1843 to 7341fab Compare July 23, 2020 12:19
@sjakobi sjakobi changed the title Trim down the extra-source-files Remove unnecessary extra-source-files Jul 23, 2020
@sjakobi sjakobi marked this pull request as ready for review July 23, 2020 12:19
@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Jul 23, 2020

Sorry, this PR must be followed actively both practically by hands and to get the change lists, so I could not follow and help with it.

Maybe some comments may have been beneficial notes into the future, what should be addressed, upstream link for test error of #672 (comment).

Also to understand the history of it, what was done - having more commits: what was removed (by:

grep 'data/nix/' old.cabal | sort --stable > old.txt
grep 'data/nix/' new.cabal | sort --stable > new.txt

# Commit 1. What was removed:
comm -23 old.txt new.txt

# Commit 2. What was added:
comm -13 old.txt new.txt

# Commit 3. Sort:
# After first two done - what is left are the sort changes.

I and others just can not follow PR with one commit with this much movement of lines around, we just can not see what was removed, what was added, and what was sorted. That is beneficial but pretty much optional. This diff is a very good basic case example of why to prefer preserving more granular commit history in the project.

The main thing that Cabal file and package are cleaned, includes what is needed and consistent.

Overall this is great change 👍

@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Jul 27, 2020

Ok. Lets move along.

@Anton-Latukha Anton-Latukha merged commit 695a47f into master Jul 27, 2020
@Anton-Latukha Anton-Latukha deleted the sjakobi/extra-source-files branch September 13, 2020 10:53
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.

2 participants