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

Fix #98209. Test all agda packages #98214

Open
wants to merge 4 commits into
base: master
from

Conversation

@turion
Copy link
Contributor

@turion turion commented Sep 18, 2020

Motivation for this change

Test all agda packages

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.

@alexarice This is WIP still.

@turion
Copy link
Contributor Author

@turion turion commented Sep 18, 2020

Currently rebased onto #98199 to get a feeling whether this is feasible.

@alexarice
Copy link
Contributor

@alexarice alexarice commented Sep 18, 2020

I'm not sure this achieves what you want in that I don't think these tests will be ran by ofBorg when you update standard-library

@turion
Copy link
Contributor Author

@turion turion commented Sep 18, 2020

Yes, true :/ Do you have an idea how to go about this?

@alexarice
Copy link
Contributor

@alexarice alexarice commented Sep 18, 2020

Disclaimer that I am not sure about this but this is what I believe happens:

  • When you submit the PR, by default ofBorg just checks that the nix evaluates (i.e. doesn't build anything)
  • If the user is recognised(?) by ofBorg (via putting in a pr to ofBorg) and the pr title has a certain format, or if someone types ofBorg build (but actually tagging ofBorg), it tries to build the package and runs any tests contained in passthru.tests

However the CI checks have looked a bit different recently, and I'm not sure if there is a detailed description of what they do somewhere

@turion
Copy link
Contributor Author

@turion turion commented Oct 19, 2020

@alexarice Does that mean this is still useful? Maybe one of us can get ofBorg rights to trigger the build.

@alexarice
Copy link
Contributor

@alexarice alexarice commented Oct 19, 2020

I have ofBorg rights, that is not the problem. The problem is that as I understand it you want:
PR put in for standard library -> ofBorg tests all downstream packages.

The test you have added does not do this. It adds checking all agda packages to the agda test, which can only be run manually.

@turion
Copy link
Contributor Author

@turion turion commented Oct 19, 2020

Right now it's fine for me if the packages are built either automatically or by triggering ofBorg manually. These PRs are still rare enough that we'll always notice. I'm fine with going with the solution that is most likely to be merged.

I could even imagine an allPackages derivation that simply has all agda packages as build inputs and does nothing otherwise, and when reviewing one can simply build that locally to verify that nothing was broken.

@alexarice
Copy link
Contributor

@alexarice alexarice commented Oct 19, 2020

Maybe there is a standard way to check for breakages in downstream packages?

@alexarice
Copy link
Contributor

@alexarice alexarice commented Oct 19, 2020

I just don't feel this test is checking if the agda infrastructure is broken which shouldn't be the case if one leaf package is a bit dodgy

@turion
Copy link
Contributor Author

@turion turion commented Oct 19, 2020

Maybe there is a standard way to check for breakages in downstream packages?

How is it done in Haskell?

@alexarice
Copy link
Contributor

@alexarice alexarice commented Oct 19, 2020

They update the whole package set with cabal2nix at one time and then I guess build the whole thing manually and see what packages broke though I am not an expert

@turion
Copy link
Contributor Author

@turion turion commented Oct 19, 2020

Ok, we can't quite copy that workflow yet (see #87903 though for an initial idea how to do that). How about the easiest way, just make a set allUnbrokenPackages, document it, and we can remind people on PRs to build it locally in order to ensure that there are no breakages?

@alexarice
Copy link
Contributor

@alexarice alexarice commented Oct 19, 2020

Does anything else do this?

@turion
Copy link
Contributor Author

@turion turion commented Oct 19, 2020

Does anything else do this?

I don't know, I'm not really involved in any other language frameworks.

@alexarice
Copy link
Contributor

@alexarice alexarice commented Oct 19, 2020

I'm not really against it, just seems odd to have a meta-package in the package set, really feels like it should be a test

@turion
Copy link
Contributor Author

@turion turion commented Oct 19, 2020

Understandable.

So a separate test? In nixos/tests/agda/allUnbrokenPackages.nix? How would it look like? It wouldn't do anything in the body, it just has all unbroken packages as dependencies?

@alexarice
Copy link
Contributor

@alexarice alexarice commented Oct 19, 2020

I'm afraid I don't really know enough about testing

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 19, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-test-all-agda-packages/9545/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.