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

Add support for meta packages resp. packages with subpackages #2267

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@sebasguts
Copy link
Contributor

sebasguts commented Mar 19, 2018

even if a PackageInfo.g is found already. This would allow
meta packages, i.e., folders containing more than one package,
to be distributed with GAP.

Currently, this is WIP belonging to #2138. It currently breaks the loading of GBNP, which could be easily fixed by renaming version/PackageInfo.g in the package into something like version/PackageInfo_template.

@ChrisJefferson

This comment has been minimized.

Copy link
Contributor

ChrisJefferson commented Mar 19, 2018

I like looking in subdirectories, but not if they contain a PackageInfo.g.

In particular, all packages which use the standard 'gh-pages' config have gh-pages/PackageInfo.g inside their directories.

@sebasguts

This comment has been minimized.

Copy link
Contributor

sebasguts commented Mar 19, 2018

True. What would you suggest instead? Check if there is also an init.g and read.g.

@ChrisJefferson

This comment has been minimized.

Copy link
Contributor

ChrisJefferson commented Mar 19, 2018

That seems like a seperate issue, which we could do generally.

I don't really understand why you really want to put packages inside packages, as opposed to having a normal directory, which contains packages in subdirectories (and could also still contain a readme, and such things, of course, just not a PackageInfo.g)

@sebasguts

This comment has been minimized.

Copy link
Contributor

sebasguts commented Mar 19, 2018

Well, those meta-packages should have a versioning, and some information about Authors, etc. Best thing would be a PackageInfo.g, or maybe a new MetapackageInfo.g?

@ChrisJefferson

This comment has been minimized.

Copy link
Contributor

ChrisJefferson commented Mar 19, 2018

I'm not sure what GAP knowledge GAP would have about these metapackages. This sounds like it would need planning. What should a metapackage do?

If you just want a package that loads everything in your metapackage, you can still just make that a normal package, and put it into your directory of packages.

Remember that (without significant other changes) if two metapackages contain the same package, GAP will just load the one of highest version (I don't know how GAP breaks ties.. I'd guess either first or last one found?)

@sebasguts

This comment has been minimized.

Copy link
Contributor

sebasguts commented Mar 20, 2018

This sounds like it would need planning. What should a metapackage do?

I think so. So, here is the backstory, along with the stuff I want to do:

Homalg and CAP both consists of multiple packages. Currently, those packages are in one git repo, but need to be shipped separately to GAP. This causes issues:

  • Incompatible versions
  • Complicated release process
  • No consistent versioning (Technically, for computations with CAP to be reproducible, one would have to specify 4 package versions, and for homalg aroung 10).

I think it would be very desirable to be able to ship those projects as one tarball to GAP, and I think one of the most sensible ways (which keeps the modularity of the projects) would be to have some kind of meta-package.

For CAP, this would be called CAP_project. In my opinion, the meta package should have the following features:

  • a version
  • authors
  • a released tarball
  • and it should be placed in a folder in the GAP package directory.

For myself, a proper PackageInfo would not be necessary, just some file that holds some of the information, e.g.,

  • Version
  • Release-tarball URL
  • Authors
  • Issue tracker URL
  • Maybe a list of packages in the meta-package, probably also with versions.

I am not sure if this would have to be in a PackageInfo, this was suggested by @alex-konovalov to be conform with the current pick up and release process.
@alex-konovalov could you comment on this?

@alex-konovalov alex-konovalov self-requested a review Mar 22, 2018

@alex-konovalov

This comment has been minimized.

Copy link
Member

alex-konovalov commented Mar 22, 2018

@sebasguts will look. Put myself as reviewer to keep it in todo list.

@fingolfin

This comment has been minimized.

Copy link
Member

fingolfin commented Mar 29, 2018

@sebasguts you give a list after "the meta package should have the following features"; then you give a different list right after, which I found a bit confusing... Perhaps the first list is for required fields, the second for "optional" ones?!?

Anyway, II think that at least from a technical point of view, it would be easiest to get this going if we reuse the existing PackageInfo.g system. To avoid the problems with that, I'd add a new optional IsMetaPackage field into the PackageInfo record (with default value false, obviously). Then, traverse into subdirectories of a package only if IsMetaPackage is set to true. This means that it can only be done later in the function, when we already are reading the PackageInfo.g files. But no problem, we can simply put the code iterating over subdirs into a subfunction and then call that in more than one place.

I would also consider adding a IsSubpackage entry into the bundled packageinfo files... or perhaps better, a ParentPackage := NAME entry? Only question is whether to ask the package authors to do that (easy to implement; but error prone, as it can easily be forgotten, or be wrong in other ways). Or do it automatically -- makes the loading code in GAP yet again more complicated, but wouldn't be that hard...
Thinking about it, I am tempted to strongly suggest the latter course, and up it by one: iterate over each subpackage, and insert a ParentPackage := parent_pkg_info_rec entry, so that it is truly linked to the precise parent package; and conversely, add to the meta package a ChildPackages := [ child1_info_rec, child2_info_rec, ...] list. Then, we could use that when loading a meta package to get the precise info files. This would prevent any kind of version mismatch, at least when the user does a LoadPackage("CAP_project"); if they load individual sub packages, things can still go wrong, but IMHO that's OK.

Anyway, going with PackageInfo.g for the meta package means that GAP will "see" the packages distributed this way right away, with relatively few changes needed. The main remaining problem then is to determine how the distribution tooling will interact with that, and how/where to change it. Some thoughts:

  • for fetching package updates, we'd only want the meta package listed in the currentPackageInfoURLList. This shouldn't be hard to ensure, might even happen automatic without any change (as the existing tooling probably does not descend into the tarball looking for further PackageInfo.g files)

  • package list on the GAP website: Do we want an entry for all subpackages there, or just for the meta package? My guess is (which might be totally wrong) that w/o further changes, only the meta package would be shown there. So that code at least may need to be taught to iterate over subpackages. But (again without actually having looked at the code yet), I'd hope that this is a relatively easy matter.

  • is anything else potentially affected? @alex-konovalov probably knows this best by far, so I hope he'll chime in when he finds some time.

In closing, I really think that many fields in the meta package can make sense; e.g. the people/person fields can be used to specify a central maintainer team, like "the homalg team" with a support email address; there could be a meta package documentation, which gives an overview of all packages it bundles, with links to their documentation; etc.; of course we can forbid certain fields for it, but I'd not do that unless we find a specific reason to do so. We could (should?), however, consider making more currently required fields optional for meta packages (which would be easy to implement with the IsMetapackage boolean field)

@ChrisJefferson

This comment has been minimized.

Copy link
Contributor

ChrisJefferson commented Mar 29, 2018

One thing to consider with metapackages -- how to implement BuildPackages.sh. We will want to look for packages inside metapackages, but not inside non-metapackages. It would make things considerably more complicated if we had to use GAP to decide where to look for packages to build, and fragile (but possible) if we tried looking for IsMetaPackage := true using (say) grep, as we might not match the string exactly (although if we used a multiline grep, we'd probably be able to get almost any valid GAP).

@fingolfin

This comment has been minimized.

Copy link
Member

fingolfin commented Mar 29, 2018

I am not so worried about that. BuildPackages.sh is a hacked up crutch in the end whatever we do (no offense to anybody who invested effort in it, but that's how I view it).

So, from a purely pragmatic point of view, I don't think a grep for IsMetaPackage := true (with some regex for the whitespace) would be a problem at all. Sure, some meta packager could do something crazy to obfuscate it, or a non-meta package could include it say hidden away in a comment) just to annoy us -- but then, so what? It'll be noticed immediately by package tests, and we'll just reject such package updates.

Of course this is not a good long-term solution, but neither is BuildPackages.sh to start with. Long term, I want a toll which I can just point at an URL and say gap-pkg-manager install https://PACKAGE.homepage. And similar for updating a package, or just rebuilding it.

BTW, using GAP to parse those PackageInfo.g files from BuildPackages.sh also doesn't sound to bad to me -- I do this in the release tools, after all, and so far nobody complained much? If you want to avoid the overhead of starting GAP N times when building N packages, no problem: once the list of package is clear, we can start GAP once, scan all PackageInfo.g files, and determine which are meta packages; then recursively scan the subpackages etc.
We might want to do that on the road to the for the future better tool anyway, in case we plan to include a way to specify build instructions inside PackageInfo.g.

Or perhaps those build instructions will go into a new file, say gap-build.json. But if we do that, then this file might as well indicate whether this is a meta packages, too (or it could just list subpackages that need compilation, if any, to save us one step).

@alex-konovalov

This comment has been minimized.

Copy link
Member

alex-konovalov commented Jun 7, 2018

As I said in #2138 :

My concern at the moment is that any changes which will make possible to build packages and to load packages located in subdirectories should also work with the existing package redistribution mechanism, or allow its adaptation.
If a meta-package would have PackageInfo.g file and will have packages in subdirectories, then the current system for checking for package update would not require any modification at all. I will simply add new package for the redistribution, and will remove from the redistribution all packages that will now be part of this meta-package.
Scripts that produce auto-generated pages with package overviews for the GAP website would require some adjustments, but that does not seem to be an obstacle.

I think we should continue discussion at #2138 before revising this PR. One of the questions is which new field(s) in PackageInfo.g files will be needed to reflect this. For example, there could be an entry in PackageInfo.g which lists names of subpackages forming the project, for example

PackageComponents:=["ExamplesForHomalg", "GaussForHomalg", "GradedModules",...],

Then package loading will traverse subdirectories ("ExamplesForHomalg-2017.09.02", "GaussForHomalg-2018.02.05" etc.) , but will assume that they contain packages that have to be loaded if and only if they have PackageInfo.g file for a package whose name matches the one in PackageComponents. This will thus allow to reject gh-pages directory (since it will have PackageInfo.g which will provide a name of a meta-package, but not the one from PackageComponents).

@alex-konovalov
Copy link
Member

alex-konovalov left a comment

This needs further discussion (perhaps under #2138) and changes that will result from it.

sebasguts added some commits Sep 18, 2018

Refactor InitializePackagesInfoRecords
Moved the loop that looks for PackageInfo.g to
FindPackageInfosInSubdirectories
and the loop that adds package infos to the global list to
AddPackageInfos
Added the possibility to add metapackages.
Metapackages form a type of package that serve
as a container for several packages distributed
in one project, see for example homalg.

@sebasguts sebasguts force-pushed the sebasguts:meta_packages branch from 904b2d4 to 2fe1e03 Sep 18, 2018

@fingolfin fingolfin changed the title Made InitializePackagesInfoRecords look into subdirs of packages Add support for meta packages resp. packages with subpackages Sep 18, 2018

@fingolfin
Copy link
Member

fingolfin left a comment

It would be great if there was a test for this: Note that we already have tst/mockpkg/; I'd imagine that we could just add tst/mockmetapkg/ next to it, and use it for tests in a similar fashion

Show resolved Hide resolved lib/package.gd Outdated
Show resolved Hide resolved lib/package.gi
Show resolved Hide resolved lib/package.gi Outdated
Show resolved Hide resolved lib/package.gi Outdated
Show resolved Hide resolved lib/package.gi Outdated
Show resolved Hide resolved lib/package.gi Outdated
Show resolved Hide resolved lib/package.gi Outdated
Show resolved Hide resolved lib/package.gi Outdated
Show resolved Hide resolved lib/package.gi Outdated
Show resolved Hide resolved lib/package.gi Outdated

sebasguts added a commit to sebasguts/gap that referenced this pull request Sep 19, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #2267 into master will decrease coverage by 7.04%.
The diff coverage is 72.54%.

@@            Coverage Diff             @@
##           master    #2267      +/-   ##
==========================================
- Coverage   83.14%   76.09%   -7.05%     
==========================================
  Files         628      480     -148     
  Lines      315831   241030   -74801     
==========================================
- Hits       262589   183414   -79175     
- Misses      53242    57616    +4374
Impacted Files Coverage Δ
lib/package.gi 70.68% <72.54%> (-2.6%) ⬇️
lib/trans.gd 0% <0%> (-99%) ⬇️
lib/pperm.gd 0% <0%> (-97.02%) ⬇️
lib/rws.gi 14.7% <0%> (-56.59%) ⬇️
lib/set.gi 9.09% <0%> (-56.4%) ⬇️
lib/rvecempt.gi 37.5% <0%> (-52.3%) ⬇️
lib/mgmideal.gi 11.39% <0%> (-48.5%) ⬇️
lib/float.gd 45.45% <0%> (-48.17%) ⬇️
lib/memory.gi 2.59% <0%> (-44.87%) ⬇️
lib/reread.g 44.82% <0%> (-43.75%) ⬇️
... and 669 more
## of info records.
##
DeclareGlobalFunction( "AddPackageInfos" );

This comment has been minimized.

@alex-konovalov

alex-konovalov Sep 19, 2018

Member

The comment says AddPackageInfo without s. Also, it's not clear to me what is a "file list". It it a list of files? Or a file containing a list? Or something else? What's the use case for this function?

Show resolved Hide resolved lib/package.gi Outdated
Show resolved Hide resolved lib/package.gi
Show resolved Hide resolved lib/package.gi

PR revisiting

@alex-konovalov alex-konovalov self-requested a review Sep 19, 2018

@sebasguts sebasguts force-pushed the sebasguts:meta_packages branch from fafd75f to 3c228e7 Sep 19, 2018

sebasguts added a commit to sebasguts/gap that referenced this pull request Sep 19, 2018

@sebasguts sebasguts force-pushed the sebasguts:meta_packages branch from 3c228e7 to ec2c650 Sep 20, 2018

sebasguts added a commit to sebasguts/gap that referenced this pull request Sep 20, 2018

@fingolfin

This comment has been minimized.

Copy link
Member

fingolfin commented Sep 20, 2018

@sebasguts I think the biggest change I am missing is the addition of tst/mockmetapkg

@alex-konovalov

This comment has been minimized.

Copy link
Member

alex-konovalov commented Sep 28, 2018

I'm still observing this problem:

* Metapackage - new version 0.1 discovered!!!
  ============================================
  Getting new archives from 
  https://github.com/sebasguts/MetaPackage/releases/download/MetaPackage-0.1/M\
etaPackage-0.1[ ".tar.gz" ]
  unpacking MetaPackage-0.1.tar.gz
#E  component `PackageDoc' must be bound to a record or a list of records
  ERROR (Metapackage): validation of the info file not successful! SKIPPING!!!
@sebasguts

This comment has been minimized.

Copy link
Contributor

sebasguts commented Sep 28, 2018

Where do you get this error from? Is it somewhere in GAP, or from some validating script you use?

I personally would prefer if MetaPackages would not be required to have a documentation on their own.

@alex-konovalov
Copy link
Member

alex-konovalov left a comment

I agree that for a meta-package documentation should be optional, but you need to update your PR to use TestOption or TestMandat dependently on the value of hasSubpackage.

TestOption( record, "PackageWWWHome", IsURL, "a string started with http://, https:// or ftp://" );
else
TestMandat( record, "PackageWWWHome", IsURL, "a string started with http://, https:// or ftp://" );
fi;
if TestMandat( record, "PackageDoc",

This comment has been minimized.

@alex-konovalov

alex-konovalov Sep 28, 2018

Member

The error

#E  component `PackageDoc' must be bound to a record or a list of records
  ERROR (Metapackage): validation of the info file not successful! SKIPPING!!!

is produced by ValidatePackageInfo here.

@fingolfin
Copy link
Member

fingolfin left a comment

@sebasguts May I suggest that you make a new PR which only contains the first commit in this PR? That commit just refactors things, which IMHO is useful by itself, and could be merged quickly; then the rest of this PR gets smaller and easier to review.

But that's only a suggestion, not required at all.

Show resolved Hide resolved lib/package.gi Outdated
Show resolved Hide resolved lib/package.gi Outdated

@sebasguts sebasguts force-pushed the sebasguts:meta_packages branch from ec2c650 to 3be4737 Oct 11, 2018

@@ -159,6 +159,17 @@ DeclareGlobalFunction( "RECORDS_FILE" );
DeclareGlobalFunction( "SetPackageInfo" );


#############################################################################
##
#F AddPackageInfo( files, pkgdir, ignore, metapackages... )

This comment has been minimized.

@fingolfin

fingolfin Nov 10, 2018

Member
Suggested change Beta
#F AddPackageInfo( files, pkgdir, ignore, metapackages... )
#F AddPackageInfos( files, pkgdir, ignore, metapackages... )

if IsBound( record.HasSubpackage ) and record.HasSubpackage = true then
for name in Set( DirectoryContents( record.InstallationPath ) ) do
dir := Directory( record.InstallationPath );

This comment has been minimized.

@fingolfin

fingolfin Nov 10, 2018

Member

Move this line out of the for loop

#F FindPackageInfosInSubdirectories( pkgdir, name )
##
## Finds all PackageInfos in subdirectories of directory name in
## directory pkgdir, returns a list of their paths.

This comment has been minimized.

@fingolfin

fingolfin Nov 10, 2018

Member

I find this description difficult to understand. How about this:

This function tests if has a subdirectory named `; if so, it iterates over all subdirectories
of , and returns a list of those which contain a PackageInfo.g files.

But thinking some more about it, I think it would be even better to replace this function by one that only takes one arguments, path, and which returns all subdirs of path that contain a PackageInfo.g. That is much easier to understand and explain.

Then, add another function which returns all subdirs of a path, if any; this function has more users, after all, even in this PR (in fact, even in this very function :-) ). It could look like this:

SubdirectoriesOfPath := function(path)
  local name, file, subs;
  subs := [];
  path := Directory(path);
  for name in Set(DirectoryContents(path)) do
    if name in [ ".", ".." ] then continue; fi;
    file := Filename(path, name);
    if IsDirectoryPath(file) then Add(subs, file); fi;
  od;
  return subs;
end;
Add( files,
[ file, Concatenation( name, "/", subdir ) ] );
fi;
od;

This comment has been minimized.

@fingolfin

fingolfin Nov 10, 2018

Member

This loop could now also be simplified, e.g. to something like this:

  files := Filtered(SubdirectoriesOfPath(pkgpath), dir -> fail <> Filename([Directory(dir)], "PackageInfo.g"));
#F EnrichMetapackageRecord( subpackagerecord, metapackagerecord )
##
## Enriches the PackageInfo of the metapackage with information from the
## Subpackage:

This comment has been minimized.

@fingolfin

fingolfin Nov 10, 2018

Member
Suggested change Beta
## Subpackage:
## subpackage:

@fingolfin fingolfin added this to the GAP 4.11 milestone Dec 27, 2018

@fingolfin

This comment has been minimized.

Copy link
Member

fingolfin commented Dec 27, 2018

I'd like to get this into GAP 4.11, but besides a conflict that requires a rebase, there are also various comments that need to be addressed. And then I guess we need some documentation updates, and possible changes to the GapWWW and gap-distribution repositories...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment