Support several package instances with same version #3668

ThomasBreuer · 2019-09-19T11:17:59Z

AddPackageInfos does no longer ignore package installations for which one instance with the same version number has already been found.
InitializePackagesInfoRecords sorts the info records stably, in order to achieve that the loadable instance gets chosen in the first possible root directory.
DirectoriesPackagePrograms and DirectoriesPackageLibrary now search according to the installation path, not to the version number.

This pull request is intended to fix issue #3663.

Two related questions:

The manual example for DirectoriesPackagePrograms shows an output of length larger than 1.

  gap> DirectoriesPackagePrograms( "nq" );
  [ dir("/home/gap/4.0/pkg/nq/bin/x86_64-unknown-linux-gnu-gcc/64-bit/"),
    dir("/home/gap/4.0/pkg/nq/bin/x86_64-unknown-linux-gnu-gcc/") ]

Has this ever been possible?

The code of AddPackageInfos contains a FIXME comment that mentions the (closed) issue Immutable GAPInfo.PackagesInfo records: Problems for JuliaInterface and float #2568. Should we remove the FIXME, or should MakeImmutable perhaps be called in general, not just in the IsHPCGAP situation?

Text for release notes

If several instances of a package, with the same version number, are available in the root directories, and if the first such instance cannot be loaded (because a compiled module is missing or does not fit) then the other instances are considered for being loaded.
In earlier versions, only the first instance of a given package version was considered.

fingolfin · 2019-09-19T13:34:05Z

Yes, in older GAP versions, two paths in DirectoriesPackagePrograms like in the example you quote were possible, because there was a second "architecture" GAPInfo.ArchitectureBase. Actually, that code was added by you 2011-04-21, and removed by me 2017-09-21 in commit c67e018 -- clearly I overlooked those examples, which should be adjusted.

However, I actually always thought that the intention behind the code in DirectoriesPackagePrograms was all along to support multiple copies of the same version of a package, by simply returning a list containing all the bin dirs of each copy -- well, at least each that contains a matching bin/ARCH subdir...

lib/package.gi

fingolfin · 2019-09-19T13:23:17Z

lib/package.gi

    # We are not allowed to call
    # `InstalledPackageVersion', `TestPackageAvailability' etc.
    info:= PackageInfo( name );
    if IsBound( GAPInfo.PackagesLoaded.( name ) ) then
      # The package is already loaded.
-      version:= GAPInfo.PackagesLoaded.( name )[2];
+      ppath:= GAPInfo.PackagesLoaded.( name )[1];


What does ppath stand for? "package path"? I first thought it was a typo. So, optional suggestion: revert to path, or use a more expressive name, like pkg_path?

I wanted the code of DirectoriesPackageLibrary and DirectoriesPackagePrograms to be similar, and path was already used in one of them.
I can choose another variable name, but I think this discussion is not useful.

fingolfin · 2019-09-19T13:26:11Z

lib/package.gi

-      version:= info[1].Version;
+      # Take the installed package with the highest version
+      # that has been found first in the root paths.
+      ppath:= info[1].InstallationPath;


So, that's a semantical change now, isn't it? Before we took all packages with the same version, and for each took its path. Now, you only take the first one, and ignore the rest.

Is that intentional? Why? Also, is this covered by the PR description? Maybe it is, but I don't quite understand all in there, either. E.g. what does this mean:

DirectoriesPackagePrograms and DirectoriesPackageLibrary now search according to the installation path, not to the version number

I think this confused me because "search" is ambiguous here. When reading that comment, I am thinking about loading packages, and then "search" immediately leads me to think about "searching the pkg dirs in the GAP roots for PackageInfo.g files".

Anyway, I really had to look at the code to understand what is going on, and even now, I am not sure I fully understand. I need to look at that code in a wider context, I think (i.e., open up the file, and read)

I do not understand the comment.
The list info contains all info records for the package in question, sorted by decreasing version number (and in the new version, where several records for the same version can exist, also sorted according to the ordering of the root paths where they were found).
In the old version, we fetch the version number and later look up the (unique) info record with this version number in order to fetch its path.
In the new version, we fetch the path directly.
(Perhaps the reason for the confusion is the for loop in the old version, which did not find more than one entry to add because there was only one package for the given version.)

Yes, the confusion is: why is there only one record for each given version? My (apparently wrong) impression was that the plan with this patch was to add all copies of a given package we find to our records, even those with identical version; but keep them sorted in the order we find them; and then when looking for binaries, we search all copies of the latest version. But it seems I misunderstood the intentions here?

And I still think that the phrase "search according to the installation path" is completely unclear. You now wrote "sorted according to the ordering of the root paths where they were found" which makes much more senes to me.

@fingolfin Thanks for the explanations.
As I had written in the description, the idea is to admit instances of a package with the same version numbers, in the sense that the package is regarded as loadable if there is one instance which is loadable.
This means in particular that it is not supported to "distribute the parts of a package to different root directories", for example to have a valid package binary only in one directory, some GAP functions of the package only in a second directory, and some package data only in a third directory.

(Concerning the test failure due to decreased coverage:
The coverage decreases when one removes some useless but covered code lines.
And when one changes or moves an uncovered line then apparently this counts as the introduction of a new uncovered line.
Very helpful.)

The coverage failure does not block merging, only actual test suite failures do (this is configured in the "branch protection rules"). The PR status says "Merging is blocked" not because of the "failing" coverage, but because there is no approving review yet.

lib/package.gi

ThomasBreuer · 2019-09-19T14:29:48Z

@fingolfin Concerning the remark

However, I actually always thought that the intention behind the code in DirectoriesPackagePrograms was all along to support multiple copies of the same version of a package, by simply returning a list containing all the bin dirs of each copy -- well, at least each that contains a matching bin/ARCH subdir...

Multiple instances of the same version of a package were not supported up to now,
and I think that it would be asking for trouble if GAP could collect pieces of data from different instances.

coveralls · 2019-09-19T18:36:19Z

Coverage decreased (-0.001%) to 84.441% when pulling df57dd8 on ThomasBreuer:TB_package_loading into fcd1f51 on gap-system:master.

fingolfin

Thanks for the PR and the explanations, this looks fine to me, now that I understand better what the intentions here are :-).

- `AddPackageInfos` does no longer ignore package installations for which one instance with the same version number has already been found. - `InitializePackagesInfoRecords` sorts the info records stably, in order to achieve that the loadable instance gets chosen in the first possible root directory. - `DirectoriesPackagePrograms` and `DirectoriesPackageLibrary` now search according to the installation path, not to the version number. These changes are intended to fix issue gap-system#3663.

- changed the documentation of `DirectoriesPackagePrograms` and `DirectoriesPackageLibrary`, such that it is clear that one gets either no directory or just one - fixed the (not testable) manual example for `DirectoriesPackagePrograms` - and beautified the name of a local variable

mohamed-barakat · 2019-10-18T12:11:36Z

Thanks to both of you @ThomasBreuer @fingolfin.

ThomasBreuer requested a review from fingolfin September 19, 2019 12:46

fingolfin reviewed Sep 19, 2019

View reviewed changes

fingolfin approved these changes Sep 24, 2019

View reviewed changes

ThomasBreuer added 2 commits September 24, 2019 16:35

ThomasBreuer force-pushed the TB_package_loading branch from fe1bd68 to df57dd8 Compare September 24, 2019 14:54

fingolfin merged commit 0b39654 into gap-system:master Oct 4, 2019

DominikBernhardt mentioned this pull request Oct 16, 2019

Problem with several root directories containing the same version of a package #3663

Closed

ThomasBreuer self-assigned this Feb 17, 2021

ThomasBreuer deleted the TB_package_loading branch February 17, 2021 13:37

ThomasBreuer added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 17, 2021

ThomasBreuer removed their assignment Feb 17, 2021

olexandr-konovalov mentioned this pull request Feb 19, 2021

Simplify the creation of release notes #4257

Closed

olexandr-konovalov mentioned this pull request Mar 23, 2021

Python script to get an overview of changes with PyGithub #4277

Merged

fingolfin changed the title ~~support several package instances with same version~~ Support several package instances with same version Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support several package instances with same version #3668

Support several package instances with same version #3668

ThomasBreuer commented Sep 19, 2019

fingolfin commented Sep 19, 2019

fingolfin Sep 19, 2019

ThomasBreuer Sep 19, 2019

fingolfin Sep 19, 2019

ThomasBreuer Sep 19, 2019

fingolfin Sep 21, 2019

ThomasBreuer Sep 23, 2019

fingolfin Sep 24, 2019

ThomasBreuer commented Sep 19, 2019

coveralls commented Sep 19, 2019 •

edited

Loading

fingolfin left a comment

mohamed-barakat commented Oct 18, 2019

Support several package instances with same version #3668

Support several package instances with same version #3668

Conversation

ThomasBreuer commented Sep 19, 2019

Text for release notes

fingolfin commented Sep 19, 2019

fingolfin Sep 19, 2019

Choose a reason for hiding this comment

ThomasBreuer Sep 19, 2019

Choose a reason for hiding this comment

fingolfin Sep 19, 2019

Choose a reason for hiding this comment

ThomasBreuer Sep 19, 2019

Choose a reason for hiding this comment

fingolfin Sep 21, 2019

Choose a reason for hiding this comment

ThomasBreuer Sep 23, 2019

Choose a reason for hiding this comment

fingolfin Sep 24, 2019

Choose a reason for hiding this comment

ThomasBreuer commented Sep 19, 2019

coveralls commented Sep 19, 2019 • edited Loading

fingolfin left a comment

Choose a reason for hiding this comment

mohamed-barakat commented Oct 18, 2019

coveralls commented Sep 19, 2019 •

edited

Loading