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

Add command line option --packagedirs and GAP function ExtendPackageDirectories() to make it easier to use custom packages #5873

Merged
merged 17 commits into from
Jan 6, 2025

Conversation

lgoettgens
Copy link
Contributor

@lgoettgens lgoettgens commented Dec 17, 2024

Resurrection of and thus closes #933. Work towards oscar-system/GAP.jl#1092.

Please see the above linked PRs/issues for the motivation of this change.

This currently only contains the changes from @fingolfin in 2016 and some documentation adaptions. I am still working on the cmd line option.

I noticed that for root dirs the corresponding function is called ExtendRootDirectories(), but they are stored in GAPInfo.RootPaths. In #933, there was a mix of GAPInfo.PackageDirectories and GAPInfo.PackagePaths. Do you have any preference here @fingolfin ?

fingolfin and others added 3 commits December 17, 2024 16:42
This is a first step towards allowing package directories to be
specified independently from GAP root paths.
@lgoettgens lgoettgens force-pushed the lg/PackageDirectories branch from ab3806c to 99e7acb Compare December 18, 2024 10:55
@lgoettgens
Copy link
Contributor Author

This now has a cmd line argument part for --packagedirs. It does not have the semicolon magic as -l, as it is not clear to me what this should mean in this case. Should it skip the pkg subdirs of the gap roots? But what is with roots added later using ExtendRootDirectories()?

And what is there else to do here?

doc/ref/files.xml Outdated Show resolved Hide resolved
lib/package.gi Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

Thank you for working on this! I have no preference between GAPInfo.PackageDirectories and GAPInfo.PackagePaths

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

The code looks o.k., modulo the hints that were already given.
I have added a few comments about the documentation.

lib/system.g Outdated Show resolved Hide resolved
doc/ref/files.xml Show resolved Hide resolved
doc/ref/files.xml Outdated Show resolved Hide resolved
doc/ref/files.xml Outdated Show resolved Hide resolved
doc/ref/files.xml Outdated Show resolved Hide resolved
doc/ref/files.xml Outdated Show resolved Hide resolved
doc/ref/run.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

I have suggested changes to the description of the new command line option. (I hope I got it right.)

doc/ref/files.xml Outdated Show resolved Hide resolved
doc/ref/files.xml Outdated Show resolved Hide resolved
doc/ref/run.xml Outdated Show resolved Hide resolved
doc/ref/run.xml Outdated Show resolved Hide resolved
doc/ref/run.xml Outdated Show resolved Hide resolved
doc/ref/run.xml Outdated Show resolved Hide resolved
Co-authored-by: Max Horn <[email protected]>
Co-authored-by: Thomas Breuer <[email protected]>
doc/ref/run.xml Outdated Show resolved Hide resolved
Co-authored-by: Max Horn <[email protected]>
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thank you, looks great to me!

Perhaps we can get some native speakers like @james-d-mitchell or @ChrisJefferson to have a look at the text before merging, so I'll wait a bit with that (but in the end, things can be improved in a follow-up PR, too)

doc/ref/files.xml Outdated Show resolved Hide resolved
doc/ref/files.xml Outdated Show resolved Hide resolved
doc/ref/files.xml Outdated Show resolved Hide resolved
doc/ref/files.xml Outdated Show resolved Hide resolved
doc/ref/run.xml Outdated Show resolved Hide resolved
doc/ref/run.xml Outdated Show resolved Hide resolved
@schnellecom
Copy link
Contributor

I also think that this addition is valuable, in particular during the development of GAP packages.
In that scenario: What happens, if a package is loaded through --packagedirs, then the package is changed and then loaded again? I can imagine that if the package is completely reloaded this would be helpful for testing newly implemented parts of a package in developement.

@fingolfin
Copy link
Member

Reloading packages is completely unrelated to this PR, though, and not generally supported by GAP. (Limited support for reloading individual files exists; but not for changing from where a package is loaded)

Co-authored-by: Lukas Schnelle <[email protected]>
@fingolfin
Copy link
Member

There is also a CI failure, probably a direct result of the changes in here and the solution may be to adjust the test, once we understand why it changed:

  ########> Diff in /home/runner/work/gap/gap/tst/testinstall/package.tst:
  635
  # Input is:
  Last( GAPInfo.PackagesInfo.mockpkg ).InstallationPath =
       GAPInfo.PackagesLoaded.mockpkg[1];
  # Expected output:
  true
  # But found:
  false
  ########

That said, GAP CI is currently broken (see issue #5884)

@lgoettgens lgoettgens closed this Jan 2, 2025
@lgoettgens lgoettgens reopened this Jan 2, 2025
@lgoettgens
Copy link
Contributor Author

This error only occurrs in some of the CI jobs. At least CI / testinstall - - ubuntu-latest runs this as well and succeeds. Furthermore, I cannot reproduce this locally (at lest not with ./gap tst/testinstall.g) on my Arch Linux system

@ThomasBreuer
Copy link
Contributor

I cannot reproduce this problem locally.
If we have no better idea then we could change the testfile for debugging purposes, such that we see the objects
List( GAPInfo.PackagesInfo.mockpkg, x -> x.InstallationPath ) and GAPInfo.PackagesLoaded.mockpkg.

@fingolfin
Copy link
Member

Note that in the CI setup there may be additional symlinks to/for pkg subdirs, which may affect this:

dev/ci.sh:103:    rm -f pkg && ln -sf . pkg
dev/ci.sh:320:    ln -s $SRCDIR/pkg $GAPPREFIX/share/gap/pkg

Perhaps one or both should go resp. be replaced by usage of --packagedirs?

BTW, there are also failures in the GAP.jl tests

@lgoettgens lgoettgens force-pushed the lg/PackageDirectories branch from f7380cb to 7daec4d Compare January 3, 2025 15:53
@lgoettgens
Copy link
Contributor Author

Note that in the CI setup there may be additional symlinks to/for pkg subdirs, which may affect this:

dev/ci.sh:103:    rm -f pkg && ln -sf . pkg
dev/ci.sh:320:    ln -s $SRCDIR/pkg $GAPPREFIX/share/gap/pkg

Perhaps one or both should go resp. be replaced by usage of --packagedirs?

Thanks for the hint. Indeed, the first one should use --packagedirs instead.

BTW, there are also failures in the GAP.jl tests

These are due to the way I used to interpret a packagedir both as the directory of a package and as a directory that contains packages with directories. This should be fixed in the most recent commit.

doc/ref/files.xml Outdated Show resolved Hide resolved
lib/package.gi Outdated Show resolved Hide resolved
lib/package.gi Outdated Show resolved Hide resolved
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

I applied a few minor tweaks to the documentation and even more minor ones to the code. Assuming CI passes I think this is good to go.

@fingolfin fingolfin enabled auto-merge (squash) January 6, 2025 12:45
@fingolfin fingolfin disabled auto-merge January 6, 2025 12:45
@fingolfin fingolfin enabled auto-merge (squash) January 6, 2025 12:46
@fingolfin fingolfin merged commit d2dab76 into gap-system:master Jan 6, 2025
33 checks passed
@lgoettgens lgoettgens deleted the lg/PackageDirectories branch January 6, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: new feature release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants