Skip to content

Make all root paths and package directories absolute paths - #5930

Open
lgoettgens wants to merge 9 commits into
gap-system:masterfrom
lgoettgens:lg/absolutize-paths
Open

lgoettgens wants to merge 9 commits into
gap-system:masterfrom
lgoettgens:lg/absolutize-paths

Conversation

@lgoettgens

Copy link
Copy Markdown
Member

As discussed in #5916 (comment).

This currently contains #5927; I'll rebase once that is merged.

The changes for package directories are straightforward. The only possible improvement I see for this case is to add a function that takes a Directory obj and converts its internal path into an absolute path. Instead, I just did this by unwrapping the underlying path string, converting, and re-wrapping it in a Directory.

For root paths, my main motivation to change it already in the kernel was that it was already documented in

gap/src/sysroots.h

Lines 50 to 53 in c5a9c8c

*F SyGetGapRootPaths() . . . . . . . . . return the list of root directories
**
** Returns a plain list containing absolute paths of the root directories as
** string objects.
that root paths are absolute, but this was not verified / converted before. In particular the changes in sysroots.c would be happy to be improved by comments; I am neither fluent with C nor familiar with the coding style in the gap kernel.

I verified that relative paths both in -l and --packagedirs work as expected (checked via GAPInfo.RootPaths and GAPInfo.PackageDirectories, respectively).
Note however that ./gap -l ";./" will result in one root path being there twice. But since this already happens with `./gap -l ";/absolute/path/to/current/dir/" as well, I see no reason to change it.

cc @ThomasBreuer @fingolfin

@lgoettgens
lgoettgens force-pushed the lg/absolutize-paths branch 2 times, most recently from ffb62e3 to ff9537e Compare February 10, 2025 13:11
@lgoettgens
lgoettgens marked this pull request as ready for review February 10, 2025 13:34
@lgoettgens

Copy link
Copy Markdown
Member Author

Hm, it seems a bit weird to me that this only fails for the GAP.jl jobs with macos. The other GAP.jl jobs are fine, as are the macos jobs of pure GAP...

Any ideas what's going on here?

@ChrisJefferson

Copy link
Copy Markdown
Contributor

I'll let @fingolfin have an opinion, I'm happy to merge this as I think it gets us closer to everything working, but I'm not sure what exactly is needed to fix GAP.jl on mac os (i can see the problem is with packagemanager, so the bug might well be there, and may need this to fix).

@ChrisJefferson

Copy link
Copy Markdown
Contributor

Can you try rebasing this on top of the current master, just to see if that clears up the GAP.jl issues?

@lgoettgens lgoettgens closed this Feb 14, 2025
@lgoettgens lgoettgens reopened this Feb 14, 2025
@lgoettgens

Copy link
Copy Markdown
Member Author

Can you try rebasing this on top of the current master, just to see if that clears up the GAP.jl issues?

Reopening should create a new tentative merge commit on top of the latest master to run CI on. Let's see what CI does now

@ThomasBreuer

Copy link
Copy Markdown
Contributor

Note however that ./gap -l ";./" will result in one root path being there twice. But since this already happens with ./gap -l ";/absolute/path/to/current/dir/" as well, I see no reason to change it.

I think this duplication of root directories is not really a problem but it causes unnecessary overhead at runtime when GAP tries to find a file in some root directory. For example, currently also the package directories corresponding to duplicate root directories occur several times, which has the effect that the PackagesInfo entries of packages inside the duplicate root directory occur several times.

I think it would be an improvement to turn GAPInfo.RootPaths into a duplicate free list, but to keep the original value of GAPInfo.KernelInfo.GAP_ROOT_PATHS.
I can create a separate pull request for this change, since it is independent of the changes proposed in the current pull request.

Comment thread src/sysroots.c Outdated
@fingolfin

Copy link
Copy Markdown
Member

Sorry I've been busy with other things. Regarding this and @lgoettgens other recent PR about the perfect groups, let me mention that I have a WIP PR that unifies the two root path lists in the kernel and library -- i.e. they'd be identical.

This is enabled by my recent refactoring of the GAP startup code, which allows me to split the command line argument parsing in the kernel in two stages; the first stage is sufficient to get the arguments which specify settings relevant to the GC. After this the GC can be initialized, and then the command line arguments can be scanned again. In this second round the gaproots are handled. Since the memory manager is now around, I can use GAP lists and strings instead of gnarly C code.

This has many advantages, and also lifts the hard limit of <= 16 GAP roots being supported in the kernel list of GAP roots.

However I just didn't have time to finish it all up, there are some annoying details related to workspaces that need to be ironed out. But I'll try to post at least a draft of it today.

Co-authored-by: Max Horn <max@quendi.de>
@lgoettgens

Copy link
Copy Markdown
Member Author

Sorry I've been busy with other things. Regarding this and @lgoettgens other recent PR about the perfect groups, let me mention that I have a WIP PR that unifies the two root path lists in the kernel and library -- i.e. they'd be identical.

This is enabled by my recent refactoring of the GAP startup code, which allows me to split the command line argument parsing in the kernel in two stages; the first stage is sufficient to get the arguments which specify settings relevant to the GC. After this the GC can be initialized, and then the command line arguments can be scanned again. In this second round the gaproots are handled. Since the memory manager is now around, I can use GAP lists and strings instead of gnarly C code.

This has many advantages, and also lifts the hard limit of <= 16 GAP roots being supported in the kernel list of GAP roots.

That sounds like a great idea, and would definitely make this PR a lot simpler and probably render #5938 obsolete.

However I just didn't have time to finish it all up, there are some annoying details related to workspaces that need to be ironed out. But I'll try to post at least a draft of it today.

Gentle reminder to do that :)

@fingolfin fingolfin closed this Jun 30, 2026
@fingolfin fingolfin reopened this Jun 30, 2026

@fingolfin fingolfin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should just do this, my PR #5941 is stalled, and this solves some concrete issues (e.g. Thomas also mentioned problems caused by having the same directory multiple times in the list of root dirs)

@lgoettgens

Copy link
Copy Markdown
Member Author

Hm, it seems a bit weird to me that this only fails for the GAP.jl jobs with macos. The other GAP.jl jobs are fine, as are the macos jobs of pure GAP...

Any ideas what's going on here?

This is still happening. @fingolfin could you have a look at these two failing macos tests?

Wrap realpath, or _fullpath on native Windows, in one place and use it
for GAP_realpath and the root paths.

Assisted-by: Claude Code (Fable 5.1)
The conversion sat inside the tilde-expansion loop, which returned
early when HOME was unset or empty, leaving relative root paths as
given. Also append the trailing slash with strxcat so a truncated path
cannot overrun its buffer.

Assisted-by: Claude Code (Fable 5.1)
GAP_realpath strips trailing slashes while every entry of
GAPInfo.RootPaths ends in one, so the duplicate check in
ExtendRootDirectories never matched.

Assisted-by: Claude Code (Fable 5.1)
Avoid poking at the internal representation in
ExtendPackageDirectories.

Assisted-by: Claude Code (Fable 5.1)
Assisted-by: Claude Code (Fable 5.1)
@lgoettgens

lgoettgens commented Sep 18, 2026

Copy link
Copy Markdown
Member Author

(the analysis and fixes below were prepared with help of Claude Code, Fable 5.1)

I updated this branch and pushed a few follow-ups:

  • Add a helper for realpath to not revert @fingolfin's MinGW progress.
  • Claude noticed that -l arguments were only being absolutized if the HOME env variable was set. This is now resolved.
  • There was some place where paths with trailing slashes were being compared to paths without trailing slashes. This is now resolved and has a test.

Why the GAP.jl macOS jobs fail. On macOS $TMPDIR lives under /var, which is a symlink to /private/var. GAP.jl passes its scratch directory via -l and --packagedirs; with this PR those become /private/var/..., so every InstallationPath starts with /private/var. But GAP.jl sets PKGMAN_CustomPackageDir to the unresolved /var/... string, and PackageManager decides "is this package in the user package dir" by StartsWith(InstallationPath, user_pkg_dir). That prefix test now fails, hence all the "not installed in user package directory" messages. Linux is unaffected only because its temp dir contains no symlink; I reproduced the same mismatch on Linux with a symlinked package directory.

So this PR changes behaviour for anything that string-compares GAP's stored paths against a user-supplied path through a symlink. To fix it, we would need a change in PackageManager to canonicalise the user package directory before storing it in PKGMAN_UserPackageInfo via GAP_realpath when bound (available since 4.15.0).
To shorten the cycle and not have to wait for a packagemanager release, put that into GAP.jl, etc., we could temporarily canonicalise all paths that are passed to packagemanager there already (cf oscar-system/GAP.jl#1440).

One open question. GAP_realpath returns fail for nonexistent paths, and the library currently passes that on:

  • gap --packagedirs /not/yet/there fails at startup in Directory(fail) (master fails later in the scan instead, so this only moves the error).
  • ExtendRootDirectories(["/not/yet/there"]) fails in EndsWith(fail, "/"), whereas master accepts it and DirectoriesLibrary simply skips missing roots.

The kernel keeps unresolvable root paths verbatim, so -l /not/yet/there still works. Should the library do the same (resolve what resolves, keep the rest), or raise a clear error at these three sites?

@fingolfin

Copy link
Copy Markdown
Member

There would be less of a problem if this didn't use realpath. Indeed, it can also be confusing for a user when they set one path, but then upon reviewing the list of configured path in GAP, see a different one.

Naive question: if a root path is already absolute (= starts with a /, at least on non-Mingw), why even pass it through realpath?

@fingolfin fingolfin added this to the GAP 4.17.0 milestone Sep 20, 2026
@fingolfin

Copy link
Copy Markdown
Member

Just to clarify: I am generally in favor of getting this into 4.17.0, but it would be good to minimize the impact this has for anyone not using relative paths.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants