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.

Sign up for GitHub

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

Jump to bottom

Idris packages clean ups and updates #42861

Merged
merged 12 commits into from Jul 8, 2018
Merged

Idris packages clean ups and updates #42861

merged 12 commits into from Jul 8, 2018

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jul 2, 2018 •
edited

Motivation for this change
  1. Installs Idris package documentation
  2. Have prelude and base as default dependencies, have idris as a buildInput, propagate Idris' meta.platforms to all packages
  3. Clean up Idris packages via 2.
  4. Mark some packages as broken
  5. Fix some packages that were broken (mainly due to Idris 1.3 not allowing upper case identifiers)
  6. Fix some builds via my own forks

I recommend reviewing by commit, they represent above points in order. Edit: Right now it seems that GitHub somehow messed up the order.. it's correct directly viewed on my branch

For 6., these are the upstream PRs:

This PR should not be merged for 1 week (That's what the WIP indicates), to give the above PR's time to be merged, at which point I'll replace my fork with the master version. I propose to use my fork if it takes longer than that.

Ping @brainrape @shlevy @ttuegel

Things done

All idrisPackages that aren't broken (or have a broken dependency) now build successfully.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@cnd
Copy link

cnd commented Jul 2, 2018

@infinisil you really have those packaged in nix?

@mpickering
Copy link
Contributor

Why are prelude and base always build dependencies? I didn't think that it was necessary to depend on either of them.

Otherwise, looks like lots of good improvements. Thanks!

@infinisil
Copy link
Member Author

@cnd Indeed, @brainrape did an amazing job at packaging lots of them in 23ee7c9

@mpickering It's not necessary (and there's the includePreludeBase option for when you don't need them, this is used for builtin packages), but the chance of anybody installing an Idris without those is pretty slim. Also, prelude and base are in a normal idris session the only packages loaded by default, which would be reflected by this change.

I just evaluated this: All packages depend on prelude. There are however 13 packages that don't depend on base (namely bytes, canvas, console, dom, free, hrtime, html, idrisscript, mapping, setoids, tparsec, webgl, xhr). But I still think it's okay to have it as a default dependency, the only influence this will have is a bit more closure size if somebody happens to not use base (which is included in the idris top level attribute anyways, which I think most Idris programmers will have installed). The advantage is not having to specify base for almost all packages.

@mpickering
Copy link
Contributor

I don't agree with this. Adding the special case is not worth the very small amount of work about being explicit. It will only be confusing to someone later on I think.

@infinisil
Copy link
Member Author

It is however also confusing having to specify dependencies prelude and base manually when ipkg files don't list them, an eventual idris2nix tool would be a more direct translation. Also, this corresponds to Idris --noprelude and --nobasepkgs flags.

@mpickering
Copy link
Contributor

I didn't realise that you didn't have to list them in the ipkg file, this seems quite wrong but if that's the case it seems to make sense to have them as defaults.

@brainrake
Copy link
Contributor

Looks good, thanks a lot @infinisil !
The only thing I would change is having noPrelude and noBase (or includePrelude and includeBase) instead of includePreludeBase, to keep in line with ipkg.

I'm happy to see the Nix Idris support evolving, in fact I would like to propose it as the "official" package manager for Idris at some point before we go reinventing the wheel. I'm working on improving the Nixpkgs manual section on Idris.

@infinisil
Copy link
Member Author

@brainrape Good point, I thought about that too, I changed it to noPrelude and noBase

@infinisil
build-idris-package: includePreludeBase -> noPrelude, noBase
5f620d0 
Resembles idris --noprelude and --nobasepkgs arguments
@infinisil
Copy link
Member Author

One thing I don't like is how the arguments are a negative (noPrelude instead of includePrelude or so), but I think it's alright since it corresponds to the flags. That good?

@brainrake
Copy link
Contributor

Yeah, same here. Thanks!

@infinisil
Copy link
Member Author

Waiting 2 more days for danilkolikov/categories#5 and david-christiansen/derive-all-the-instances#9

@infinisil
Copy link
Member Author

Above PR's haven't gotten any reaction in 7 days, merging now.

@infinisil infinisil changed the title [WIP] Idris packages clean ups and updates Idris packages clean ups and updates Jul 8, 2018
@infinisil infinisil merged commit 4b9985c into NixOS:master Jul 8, 2018
@infinisil infinisil deleted the fix/idris-forks branch July 8, 2018 21:24
infinisil added a commit to infinisil/nixpkgs that referenced this pull request Oct 26, 2018
@infinisil
idrisPackages.{categories,derive}: Use upstream instead of forks
41b55b3 
These forks were introduced in NixOS#42861 to
make the builds succeed. The changes have since been incorporated
upstream.
@infinisil infinisil mentioned this pull request Oct 26, 2018
Merged
9 tasks
@infinisil infinisil mentioned this pull request Nov 10, 2018
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@infinisil @cnd @mpickering @brainrake @GrahamcOfBorg