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 primgrp, smallgrp, transgrp from required packages #2589

Closed

Conversation

fingolfin
Copy link
Member

This starts work on issue #2434. Let's see what breaks, if anything.

Though we may not see any breakage without dedicated tests, at least in packages...

@fingolfin fingolfin added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jun 28, 2018
@fingolfin fingolfin added this to the GAP 4.10.0 milestone Jun 28, 2018
@olexandr-konovalov
Copy link
Member

@fingolfin thanks - just to let you know I've seen this and will run extended tests of PR using my parametrised Jenkins build - I'm specifying the number of the PR, and then id does the same as in
the nightly build for master or stable, but for this PR.

The changes are obviously correct, it's the impact of them that we need to estimate and report to authors of affected packages.

@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Jun 28, 2018

Observed problems are of different kinds:

  • Tests calling functions from these packages, e.g.

    List(AllSmallGroups(60), g -> Size(FrattiniSubgroup(g)));

  1. Tests calling library functions relying on these packages, e.g.
########> Diff in /circa/scratch/gap-jenkins/workspace/GAP-pull-request-quickt\
est/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/install/label/kovacs/GAP-pull-reques\
t-snapshot/tst/testinstall/opers/MaximalNormalSubgroups.tst:32
# Input is:
SortedList(List(MaximalNormalSubgroups(D2), StructureDescription));
# Expected output:
[ "C18", "D18", "D18" ]
# But found:
Error, the Small Groups identification is required but not installed
########
  1. Side effects e.g.:
########> Diff in /circa/scratch/gap-jenkins/workspace/GAP-pull-request-quickt\
est/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/install/label/kovacs/GAP-pull-reques\
t-snapshot/tst/testinstall/ctblmono.tst:117
# Input is:
TestMonomial( chi );
# Expected output:
rec( comment := "degree is index of Hall subgroup", isMonomial := true )
# But found:
rec( comment := "codegree is prime power", isMonomial := true )
########
  1. Warnings and break loops when packages are loaded

  2. Manual examples

  3. An of course, default tests of GAP packages specified in their PackageInfo.g files.

Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

I think one of the first modifications of this PR would be to add these three packages to the set of packages loaded by default:

default:= [ "autpgrp", "alnuth", "crisp", "ctbllib", "factint", "fga",

@olexandr-konovalov
Copy link
Member

@fingolfin one of the first modifications of this PR would be to add these three packages to the set of packages loaded by default - could you please update the PR?

@olexandr-konovalov
Copy link
Member

@fingolfin ping?

This way, at least in principle one can load GAP without them.
@codecov
Copy link

codecov bot commented Sep 2, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@d1b62ac). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #2589   +/-   ##
=========================================
  Coverage          ?   43.35%           
=========================================
  Files             ?      111           
  Lines             ?    57739           
  Branches          ?        0           
=========================================
  Hits              ?    25033           
  Misses            ?    32706           
  Partials          ?        0

@fingolfin
Copy link
Member Author

Updated as requested

@olexandr-konovalov olexandr-konovalov dismissed their stale review September 2, 2018 20:26

Dismissing review to create another review request

@olexandr-konovalov
Copy link
Member

The number of diffs is still overwhelming. I would prefer to deal with this one by one. Either there should be three separate PRs, or you can try now to remove only PrimGrp and see what happens. The majority of diffs are coming from SmallGrp, and we need to discuss how to deal with that.

@fingolfin
Copy link
Member Author

To be honest, I am not sure it's worth the bother, so I don't intend to do anything about this PR. It really was meant more as a reminder; for now I think there are far more important issues to think about.

@olexandr-konovalov
Copy link
Member

Fine, so the summary is that many problems show up, they are classified as below, and we need to discuss how to deal with them and in which priority.

  1. Tests calling functions from these packages
  2. Tests calling library functions relying on these packages
  3. Side effects (diffs when the test does not call a function from a package explicitly - less obvious)
  4. Warnings and break loops when packages are loaded
  5. Manual examples (now they run without diffs with default packages, but with errors when those three packages are missing)
  6. Default tests of GAP packages specified in their PackageInfo.g files.

The complexity for untangling this for each package may be different.

@fingolfin fingolfin removed this from the GAP 4.10.0 milestone Sep 28, 2018
@fingolfin
Copy link
Member Author

This PR has no chance to be merged anytime soon, if ever, and at the same time is trivial, so I don't see a point in keeping this open.

@fingolfin fingolfin closed this Jun 5, 2019
@fingolfin fingolfin deleted the mh/optional-group-libraries branch October 28, 2021 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants