I think there should be a flag for package removal; I do not use gap directly so I don't object to the packages being removed by default.

Thats probably a good idea. Because of the size (~1.3G) I think that flag should default to removal, do you agree?

I do agree; maybe @ChrisJefferson has an opinion that some specific GAP packages should be kept by default.

I added a flag. Its all-or-nothing. I thought about doing keepPackages ? [ "GAPDoc" "autopgrp" ] instead, but that would probably be annoying to people who just want a no compromises gap installation.

Please remind me to merge if there are no comments from other GAP users after a day or two…

Are you currently defaulting to keeping all packages?

I don't mind an option, but the default should be keeping all packages, as the number one source of issues we get with GAP is people who have installed GAP in Debian, where most of the packages are missing. It isn't well tested, as it isn't the intended use.

Also, I personally wouldn't bother with that patch. It won't be needed for the next release of GAP (which should appear shortly), and it's really only needed because SAGE uses GAP in weird ways (and packages it's own GAP anyway) -- most people don't close stderr!

Re: packages: OK, if it does cause problems for upstream, we should keep the packages by default. Maybe hydraPlatforms should be empty, if the CPU:HDD ratio for the package is low.

Re: patch: it would be nice to force Sage to not rebuild everything on its own, so I think it makes sense to include patch (@timokau is currently working on improving Sage packaging)

Are all packages in the pkg folder actually needed or are some of them optional? Here are the worst offenders:

→ du -hs pkg/* | sort -h | tail -n20                                                build-dir
8.1M	pkg/loops
8.8M	pkg/orb-4.7.6
9.8M	pkg/guava-3.13
9.9M	pkg/corelg
13M	pkg/digraphs-0.11.0
13M	pkg/fining
15M	pkg/factint-1.6.0
18M	pkg/sonata
21M	pkg/unitlib-3.3.0
24M	pkg/smallsemi-0.6.11
29M	pkg/recog-1.2.5
30M	pkg/kbmag-1.5.4
36M	pkg/carat
47M	pkg/rcwa-4.6.1
49M	pkg/ctbllib
73M	pkg/irredsol-1.4
74M	pkg/sglppow
139M	pkg/Hap1.12
229M	pkg/tomlib
245M	pkg/simpcomp

So just removing the top 5 or something would already have much of the benefit.

Re: hydraPlatforms: Rebuilding took ~7min (with tests) on my pc, so I don't think we should disable hydra build.

I think SAGE applies some other patches too? (They build a 'libgap', where they make changes to the configuration scripts and a bunch of other things, so they can link in GAP as a library).

However, the patch is fine, just hopefully we (the GAP developers) will get 4.9 released.

P.S. Thanks for getting this updated, I'd kind of forgotten about it, as I can't get NixOS to work on my current machine.

@timokau re: Hydra: I think buildfarm and cache are more size-sensitive than buildtime-sensitive, and 7 minutes for a large package doesn't sound that bad for a local build.

Yes, sage needs one patch more (one that disables loading any packages by default) so that gap and libgap work together (libgap needs to be installed in addition to gap).

But that other patch is not universally useful (might be harmful) which is why I didn't include it here. I added a gap-libgap-compatible to all-packages.nix, which fixes the version on 4r8p6 and adds that patch. I'll upstream that together with the libgap package.

@ChrisJefferson you're probably not the right person for that, but just in case you are I want to mention that the current versioning scheme is very confusing (with 4.9.0 being a beta release).

@7c6f434c I don't understand what you mean by size-sensitive. Do you mean that space is running out / is too expensive?

@timokau build machines can sometimes run out of space, and also binary cache is stored on S3 (currently without any pruning) which means that large packages become a recurring expense (while builds are a one-time expense)

I am the right person, and I agree it's been a confusing choice. There are boring practical reasons (we've hard-wired the idea of 3 digit versions, with the second digit meaning major breakage, in far too many places), but we should try to improve in future.

Also, some of those packages could possibly be shrunk, I'll look into it. One day, we'd like a proper package manager, where packages are downloaded when needed, but that's obviously a major undertaking.

@7c6f434c okay I see, I'll disable hydra build for now if you think thats better.

@ChrisJefferson so you're saying that for now all packages in the pkg folder are used and should all be kept by default?

@timokau : I'm saying the behaviour of GAP if you start deleting packages isn't well understood (unfortunately), as we don't test that way often. Certainly some packages could be deleted as they aren't loaded by default, or required by other packages, but we don't have a good list of what those packages are.

I certainly wouldn't delete anything which is auto-loaded by default.

I reverted the default to keeping all packages. I also added gap-minimal that does remove the packages and made hydraPlatforms dependent on weather the packages are kept or not.

This way other packages can depend on gap-minimal (as long as they don't need additional packages) while end-users can install the regular gap (without any surprises).

I think gap-minimal should be lowPrio

You're right, changed.

This is ready to be merged, right?

In my opinion yes and I think @ChrisJefferson's concerns are addressed as well.

This seems to have broken ofborg eval for some reason. It's complaining about not finding gap-minimal.aarch64: https://gist.github.com/GrahamcOfBorg/e5525b24da9c1e17f6814ab86c767caf

I don't know what that error means. @grahamc?

Fixed

Recently I don't understand why some PRs do not get evaluated. Here it was just WIP, though, and I didn't force an eval afterwards.

Explanation of the problem: it used platforms, but meta was not rec so it took platforms from with stdenv.lib; and meta.hydraPlatforms = stdenv.lib.platforms; doesn't work well. Now the conditional uses hydraPlatforms = … meta.platforms; which should work.

@7c6f434c Is this really fixed? Just got this: https://gist.github.com/GrahamcOfBorg/88412a68664f1431e733ccb37c9bacc8

@7c6f434c I could indeed fix the issue with your idea. However I cannot see any patch. Did you somehow forget to commit it ?

All PRs are still being evaluated. The PR was force-pushed to, and only eight minutes elapsed between the force push and the merge. At that time, 9:35, there was a sizable backlog of evaluations: https://monitoring.nix.ci/dashboard/db/ofborg?orgId=1&from=1523450834321&to=1523458316494 You can see when this merged, because every pending PR failed evaluation almost immediately.

@layus now that I look it seems I have indeed committed and pushed the patch, but to a wrong remote.

@grahamc I know it shouldn't have had time to get evaluated after the last force-push, but it wasn't getting evaluated while WIP, so I didn't wait for the evaluation when it could get triggered.

Maybe I should remove the part about not evaluating WIPs.

Sorry for causing this, I missed the rec bit. It did evaluate (build) locally though, probably hydraPlatforms isn't evaluated on a local build?

@timokau nixpkgs/maintainers/scripts/test-eval-release.sh catches some of evaluation problems (hopefully the same set as ofborg does!)