Skip to content

Conversation

@soehms
Copy link
Member

@soehms soehms commented Jan 23, 2024

This PR implements the suggestion made in #36741 (comment) :

I would say, the correct fix would be to remove both Gap packages from the all_features list and erase the corresponding optional tags in permgroup_named.py, distance_regular.pyx, abelian_aut.py, abelian_group.py and abelian_group_gap.py. If you agree I will open another PR for this.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@soehms soehms added this to the sage-10.3 milestone Jan 23, 2024
@soehms soehms marked this pull request as ready for review February 4, 2024 21:40
@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 16, 2024

This will need some input from Gap / downstream packaging experts @dimpase @orlitzky @tornaria @antonio-rojas; I have not followed the discussion on this closely.

The alternative to what is done here in the PR would be to keep the feature definitions for these packages and to change the tags from # optional - gap_packages_polycyclic to # needs gap_packages_polycyclic etc.

@orlitzky
Copy link
Contributor

I don't care what you want to call it but the tests shouldn't fail when the system gap is missing one of these packages. The only GAP packages that you can assume to be present are gapdoc, primgrp, smallgrp, and transgrp.

@tornaria
Copy link
Contributor

Could you use something like GapPackage("polycyclic", spkg="gap", type='standard') This is what you really want to say about this package: it is standard (for sage-the-distro) and it comes in spkg gap (for sage-the-distro). But you still need to test the presence of this package since system gap might not have it.

@antonio-rojas
Copy link
Contributor

I think it's OK for sage to assume some GAP packages are available if the funtionality they provide is deemed to be non-optional, the same way it assumes all standard dependencies are installed (and it's the packagers responsibility to make sure sage pulls them as dependencies). But in that case the spkg-configure script should be adapted to only accept system GAP if all required packages are available.

@soehms
Copy link
Member Author

soehms commented Feb 19, 2024

I don't care what you want to call it but the tests shouldn't fail when the system gap is missing one of these packages. The only GAP packages that you can assume to be present are gapdoc, primgrp, smallgrp, and transgrp.

Do we have CI runs that cover such cases? If yes can you give an example?

@soehms
Copy link
Member Author

soehms commented Feb 19, 2024

Could you use something like GapPackage("polycyclic", spkg="gap", type='standard') This is what you really want to say about this package: it is standard (for sage-the-distro) and it comes in spkg gap (for sage-the-distro). But you still need to test the presence of this package since system gap might not have it.

Currently GapPackage("polycyclic", spkg="gap", type='standard') causes UserWarning: Feature gap_package_polycyclic is declared standard, but it is provided by gap_package... (see #36741 (comment)). At least we should suppress this warning than. What about to introduce a new value for type for instance semi standard or indirect standard indicating a standard downstream package of a standard upstream package.

@dimpase
Copy link
Member

dimpase commented Feb 19, 2024

How about simply moving all such GAP packages from gap_packages spkg into gap spkg?

@orlitzky
Copy link
Contributor

I don't care what you want to call it but the tests shouldn't fail when the system gap is missing one of these packages. The only GAP packages that you can assume to be present are gapdoc, primgrp, smallgrp, and transgrp.

Do we have CI runs that cover such cases? If yes can you give an example?

Probably not, but those packages are what's tested in spkg-configure.m4 and in the new meson (build system for sagelib) pull request. So they're guaranteed to be there but nothing else is. I run without the others.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 19, 2024

How about simply moving all such GAP packages from gap_packages spkg into gap spkg?

Yes, that would be a solution. Or alternatively make it a separate gap_packages_standard package.

@dimpase
Copy link
Member

dimpase commented Feb 19, 2024

upstream GAP's default is in PackagesToLoad

   PackagesToLoad 
        A  list  of names of packages which should be loaded during startup. 
        For backwards compatibility, the default lists most of
        packages that were autoloaded in GAP 4.4 (add or remove packages as you like).
  
        Default:  [  "autpgrp",  "alnuth",  "crisp",  "ctbllib",  "factint",  "fga", "irredsol", "laguna", 
                            "polenta", "polycyclic", "resclasses", "sophus", "tomlib" ].  

of these, ctbllib is 73Mb, tomlib 53Mb, irredsol 27Mb . The rest don't have data, only code, and are much smaller.

OK to move them all?

@soehms
Copy link
Member Author

soehms commented Feb 19, 2024

OK to move them all?

At least it would be consequent! But I don't know if the additional volume of datasize is justified.

@tornaria
Copy link
Contributor

Could you use something like GapPackage("polycyclic", spkg="gap", type='standard') This is what you really want to say about this package: it is standard (for sage-the-distro) and it comes in spkg gap (for sage-the-distro). But you still need to test the presence of this package since system gap might not have it.

Currently GapPackage("polycyclic", spkg="gap", type='standard') causes UserWarning: Feature gap_package_polycyclic is declared standard, but it is provided by gap_package... (see #36741 (comment)). At least we should suppress this warning than. What about to introduce a new value for type for instance semi standard or indirect standard indicating a standard downstream package of a standard upstream package.

I'm confused. I thought the warning was caused by GapPackage("polycyclic", spkg="gap_packages", type='standard'). Do you get the same warning with spkg="gap"?

Since polycyclic is indeed shipped as part of spkg gap (and not gap_packages), the feature GapPackage("polycyclic", spkg="gap", type='standard') seems to accurately match the current state of things.

@soehms
Copy link
Member Author

soehms commented Feb 20, 2024

I'm confused. I thought the warning was caused by GapPackage("polycyclic", spkg="gap_packages", type='standard'). Do you get the same warning with spkg="gap"?

Sorry! I missed that detail. You are right, that should not lead to the warning!

@github-actions
Copy link

Documentation preview for this PR (built with commit b392d5b; changes) is ready! 🎉

@soehms
Copy link
Member Author

soehms commented May 23, 2025

How about simply moving all such GAP packages from gap_packages spkg into gap spkg?

@dimpase who can do that? Shall we have a follow-up issue / PR for that? Then I would just delete my branch!

@dimpase
Copy link
Member

dimpase commented May 23, 2025

I don't think we ever got around to this, so please complete this one, and we'll merge it

@dimpase
Copy link
Member

dimpase commented May 23, 2025

Ah, OK, sorry, I was not reading well.

It is fine to move all such GAP packages to be installed by gap spkg.

In case you don't see how to do this, open an issue, otherwise just another PR.

@orlitzky
Copy link
Contributor

One problem you are going to run into is that the packages in gap_packages are outdated and e.g. don't build with gcc-15. Moving them into the gap SPKG keeps them tied to the gap tarball & release schedule.

It would be much better to split them out into separate SPKGs that can be version-bumped and feature-tested independently. Most have their own upstreams and release schedules.

In any case, I don't really care what you do in sage-the-distro, so long as these tests still pass when the GAP packages aren't installed. The cohomolo and datastructures packages, for example, still can't be built on ~arch Gentoo so it's pretty convenient that they're optional.

@dimpase
Copy link
Member

dimpase commented May 24, 2025

I would rather have GAP's PackageManager take care of this.

Move to gcc-15 is a one time event, which can be addressed by e.g. a custom tarball

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.

6 participants