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

Normalize package paths to always contain a trailing slash #5178

Merged
merged 2 commits into from
Nov 25, 2022

Conversation

zickgraf
Copy link
Contributor

SetPackagePath always appends a trailing "/" to the installation path of a package while AddPackageInfos did not before this PR. This could lead to the following error:

gap> SetPackagePath( "cap", GAPInfo.PackagesInfo.cap[1].InstallationPath );
gap> LoadPackage( "CAP", false );
true
gap> ExtendRootDirectories( [ "/tmp/some/dir" ] );
gap> SetPackagePath( "cap", GAPInfo.PackagesInfo.cap[1].InstallationPath );
Error, another version of package cap is already loaded at .../lib/package.gi:1752
brk> GAPInfo.PackagesLoaded.( pkgname )[1];
".../.gap/pkg/CAP_project/CAP/"
brk> pkgpath;
".../.gap/pkg/CAP_project/CAP"

i.e. an error is signaled because the installation path differs by a trailing slash.

This PR does two things to prevent this:

  1. It normalizes the installation path in AddPackageInfos to also include a trailing slash.
  2. It uses Directory in the comparison which triggers the error.

Both changes would fix the error on their own, but I think both are sensible in general:

  1. Being consistent is always a good thing.
  2. pkgpath is a user input, so normalizing it is a good idea anyway (as users might enter paths including "~" or with or without trailing slashes).

Text for release notes

none

@ChrisJefferson
Copy link
Contributor

This seems fine to me in principle, but I'll be honest I've never used SetPackagePaths or ExtendRootDirectories. Some tests for this, perhaps in testinstall/package.tst using the existing mockpkg might work?

@zickgraf
Copy link
Contributor Author

zickgraf commented Nov 3, 2022

This seems fine to me in principle, but I'll be honest I've never used SetPackagePaths or ExtendRootDirectories.

Background: I use SetPackagePath to enforce loading packages from the user root dir in the CI, and our Julia package CapAndHomalg.jl uses ExtendRootDirectories to add its own root with our packages.

Some tests for this, perhaps in testinstall/package.tst using the existing mockpkg might work?

Done!

@zickgraf zickgraf force-pushed the normalize_package_paths branch 2 times, most recently from e64f523 to f5fe757 Compare November 3, 2022 07:58
@@ -640,5 +640,21 @@ false
gap> IsPackageLoaded("mockpkg", ">=2.0");
false

# now add `tst` as a new root directory
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not do this, nor add tst/pkg. Instead, we could create a temporary directory somewhere, then add a pkg subdir into it and into that a symlink to mockpkg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two questions:

  1. Can we rely on IO_symlink in the test? If not, I guess things might get messy.
  2. What about Windows, would IO_symlink work there? Or can we somehow exclude this test on Windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

In simple terms, you can assume everything unixy will work in Windows. I'll be sure to double check any test, but I just tried and IO_symlink works fine.

Copy link
Member

Choose a reason for hiding this comment

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

I would not rely on IO_symlink, which may not always be available (if IO is not compiled). You could simply use Exec("ln -sf FOO BAR"), or if you want to be able to detect errors, then use Process (and if anyone wants a nicer alternative, comment on #5103 ???)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a solution using Exec. If an error occurs, ValidatePackageInfo right after the Exec will fail saying that the file is not readable, so I think there is no need for an additional error check .

@zickgraf zickgraf force-pushed the normalize_package_paths branch 2 times, most recently from 4588319 to b2a4e4a Compare November 15, 2022 08:29
@fingolfin fingolfin added topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Nov 23, 2022
tst/testinstall/package.tst Outdated Show resolved Hide resolved
@fingolfin fingolfin closed this Nov 24, 2022
@fingolfin fingolfin reopened this Nov 24, 2022
@fingolfin fingolfin merged commit 41c2707 into gap-system:master Nov 25, 2022
@zickgraf zickgraf deleted the normalize_package_paths branch November 25, 2022 08:20
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants