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

dhall-json, dhall-lsp-server, dhall-yaml: unmark as broken #109126

Merged
merged 2 commits into from Jan 13, 2021

Conversation

mcwitt
Copy link
Contributor

@mcwitt mcwitt commented Jan 12, 2021

Motivation for this change

Several dhall-related packages are marked as broken, but appear to be building correctly (tested on NixOS).

Update: dhall-yaml fails to build on macOS; does this mean it should still be "broken" (even though it builds on NixOS)?

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@@ -73198,7 +73198,6 @@ self: {
description = "Convert between Dhall and JSON or YAML";
license = stdenv.lib.licenses.bsd3;
hydraPlatforms = stdenv.lib.platforms.none;
broken = true;
Copy link
Member

Choose a reason for hiding this comment

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

@mcwitt Thanks for making the effort to fix this.

Unfortunately this file hackage-packages.nix is regenerated automatically once a day based on the configuration-hackage2nix.yaml file. You'll have to remove these three packages from the broken-packages section in that file.

Once you do this, I can merge this PR in.

Copy link
Contributor Author

@mcwitt mcwitt Jan 13, 2021

Choose a reason for hiding this comment

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

Thanks! I had been confused about which files were auto-generated from Hackage. This makes sense, changed.

@cdepillabout
Copy link
Member

cdepillabout commented Jan 13, 2021

dhall-yaml fails to build on macOS; does this mean it should still be "broken" (even though it builds on NixOS)?

A lot of people that contribute to Nixpkgs don't have access to a MacOSX machine to test on, so a lot of changes are made without even seeing how they affect OSX.

If dhall-yaml just completely doesn't support MacOSX, then the "correct" solution is to add it to the unsupported-platforms section in configuration-hackage2nix.yaml.

However, I don't think dhall-yaml is necessarily platform specific, so I imagine that it shouldn't be too difficult to get it to build on MacOSX. I'd suggest not doing anything specific for this problem in this PR, and just let an interested OSX user dig into why this isn't building.

Also, if you've tried building it on OSX, maybe you could post the build log with the error you're seeing? If it is not something nixpkgs specific, it may make sense to report it upstream.

@mcwitt
Copy link
Contributor Author

mcwitt commented Jan 13, 2021

That makes sense, thanks.

The situation with dhall-yaml on macOS actually seems to be nondeterministic. On the initial attempt I got a property test failure (looks like a timeout):

Test suite expected-fail-hh-tests: RUNNING...
Expected Hedgehog Failures
  good:                                OK
      ✓ good passed 100 tests.
  rarely good:                         FAIL (expected)
      ✗ rarely good failed at tests/TestExpectedFailsHH.hs:22:7
        after 20 tests and 7 shrinks.

           ┏━━ tests/TestExpectedFailsHH.hs ━━━
        16 ┃ main = defaultMain $
        17 ┃   localOption (mkTimeout 1000000) $  -- 1s
        18 ┃   testGroup "Expected Hedgehog Failures" $
        19 ┃   [ testProperty "good" $ property $ success
        20 ┃   , expectFail $ testProperty "rarely good" $ property $ do
        21 ┃       xs <- forAll $ Gen.list (Range.linear 0 10) Gen.alpha
           ┃       │ "ab"
        22 ┃       reverse xs === xs
           ┃       ^^^^^^^^^^^^^^^^^
           ┃       │ ━━━ Failed (- lhs) (+ rhs) ━━━
           ┃       │ - "ba"
           ┃       │ + "ab"
        23 ┃
        24 ┃   -- n.b. uncomment this to observe the results of a test that was
        25 ┃   -- expected to fail but actually passes.
        26 ┃   -- , expectFail $ testProperty "surprisingly good" $ property $ success
        27 ┃
        28 ┃   , expectFail $ testProperty "giving up" $ property $ discard
        29 ┃
        30 ┃   , expectFail $ expectFail $ testProperty "the failure of a failure is my good" $
        31 ┃     property $ success
        32 ┃
        33 ┃   , expectFail $ testProperty "throws failure" $
        34 ┃     property $ fail "bad"
        35 ┃
        36 ┃   , expectFail $ testProperty "too slow" $
        37 ┃     property $ do
        38 ┃       liftIO $ threadDelay 2000000
        39 ┃       success
        40 ┃   ]

        This failure can be reproduced by running:
        > recheck (Size 19) (Seed 5642567062686654521 2315004385954815959) rarely good

    Use '--hedgehog-replay "Size 19 Seed 5642567062686654521 2315004385954815959"' to reproduce.
     (expected failure)
  giving up:                           FAIL (expected)
      ⚐ giving up gave up after 100 discards, passed 0 tests. (expected failure)
  the failure of a failure is my good: OK (unexpected) (expected)
      ✓ the failure of a failure is my good passed 100 tests. (unexpected success) (expected failure)
  throws failure:                      FAIL (expected)
      ✗ throws failure failed
        after 1 test.

        bad

        This failure can be reproduced by running:
        > recheck (Size 0) (Seed 16711140964275765960 10244931594257750943) throws failure

    Use '--hedgehog-replay "Size 0 Seed 16711140964275765960 10244931594257750943"' to reproduce.
     (expected failure)
  too slow:                            TIMEOUT (1.00s)
    Timed out after 1s

1 out of 6 tests failed (1.01s)
Test suite expected-fail-hh-tests: FAIL
Test suite logged to:
dist/test/tasty-expected-failure-0.12.2-expected-fail-hh-tests.log
Test suite expected-fail-tests: RUNNING...
Test suite expected-fail-tests: PASS
Test suite logged to:
dist/test/tasty-expected-failure-0.12.2-expected-fail-tests.log
1 of 2 test suites (1 of 2 test cases) passed.
builder for '/nix/store/zncw1gprsqm29cn2ij01s8rdl3h9hbfm-tasty-expected-failure-0.12.2.drv' failed with exit code 1
cannot build derivation '/nix/store/hvyiadbq6933n9h5arai9dqzcjbdqkjs-dhall-yaml-1.2.4.drv': 1 dependencies couldn't be built
error: build of '/nix/store/hvyiadbq6933n9h5arai9dqzcjbdqkjs-dhall-yaml-1.2.4.drv' failed

but when I reran just now it succeeded 🤷 . Maybe best to disable tests when we hit nondetermistic behavior. In any case, considering your advice above, seems like the right thing to leave this for a future PR.

@cdepillabout
Copy link
Member

@mcwitt When trying to build these packages locally, I am seeing the same failure with haskellPackages.tasty-expected-failure that you posted above.

I'd suggest first reporting this upstream on the tasty-expected-failure repo (although if you first tried to determine whether you could reproduce this without nix, that would probably be helpful to the maintainer).

Then, either disable the tests for dhall-yaml (so that tasty-expected-failure doesn't get pulled in), or disable the tests for tasty-expected-failure so that dhall-yaml can use it.

Also, feel free to just drop dhall-yaml from this PR and open up another PR in the future for it. I've confirmed that dhall-lsp-server and dhall-json compile fine, so I am happy to merge the two of them in.

@mcwitt
Copy link
Contributor Author

mcwitt commented Jan 13, 2021

@cdepillabout sounds good, thanks! I've dropped dhall-yaml from this PR; I'll do what you suggest and see if I can reproduce the error for tasty-expected-failure without Nix and report upstream, then open another PR to address the issue with dhall-yaml.

Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

Thanks, that sounds good!

@cdepillabout
Copy link
Member

This should be marked as non-broken the next time the automated script runs on the haskell-updates branch (which normally happens once a day). Here is a PR where the branch will be merged into master:
#109011

@cdepillabout cdepillabout merged commit 8eb0bdc into NixOS:haskell-updates Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants