Skip to content

Conversation

Copy link

Copilot AI commented Oct 24, 2025

This PR removes the circular and redundant GAP_ROOT_PATHS handling from the Sage codebase.

Problem

The current implementation has a circular dependency:

  1. Build time: Query GAP for its root paths → Store in GAP_ROOT_PATHS config variable
  2. Runtime: Pass those same paths back to GAP via the -l flag

This is unnecessary because GAP already knows its own root paths and doesn't need to be told them explicitly.

Changes

Build Configuration

  • src/sage/meson.build: Removed code that queries GAP at build time for its root paths
  • src/sage/config.py.in: Removed GAP_ROOT_PATHS variable definition

Runtime Configuration

  • src/sage/env.py: Removed GAP_ROOT_PATHS variable and its default value

GAP Interfaces

  • src/sage/interfaces/gap.py: Removed -l "{GAP_ROOT_PATHS}" flag when starting GAP. GAP now uses its built-in default root paths.
  • src/sage/libs/gap/util.pyx: Removed -l flag from libgap initialization. Adjusted argv indices accordingly.

Workspace Management

  • src/sage/interfaces/gap_workspace.py: Changed to query GAP dynamically at runtime for version and architecture info instead of using cached paths from build time.
  • src/sage/libs/gap/saved_workspace.py: Changed to query GAP dynamically at runtime for package paths when computing timestamps.

Benefits

  1. Eliminates circular dependency: No longer query GAP to get paths just to pass them back to GAP
  2. Simpler build process: One less configuration step during build
  3. More robust: Runtime queries have proper error handling and timeouts
  4. Cleaner code: Removes ~30 lines of unnecessary configuration code

Testing

All modified Python files pass syntax validation. The changes maintain backward compatibility - GAP will use its default root paths, which are the same paths we were previously passing to it.

Original prompt

Remove GAP_ROOT_PATHS handling in src/sage (both in python and meson files)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: tobiasdiez <5037600+tobiasdiez@users.noreply.github.com>
Copilot AI changed the title [WIP] Remove GAP_ROOT_PATHS handling in src/sage files Remove GAP_ROOT_PATHS handling in src/sage Oct 24, 2025
Copilot AI requested a review from tobiasdiez October 24, 2025 02:51
@tobiasdiez
Copy link
Owner

tobiasdiez commented Oct 24, 2025

@orlitzky GAP_ROOT_PATHS is also used for the gap workspace feature to generate the hash/timestamp that gets updated once you install a new gap package. How to handle this? (AI proposes to essentially create GAP_ROOT_PATHS at runtime in those cases)

@orlitzky
Copy link
Collaborator

gap --print-gaproot

Not losing my job to AI today. The point of the direct-file-access timestamp is to be fast, but I think gap --print-gaproot is also intended to be fast. (It doesn't load anything AFAICS.) If we really wanted to avoid the subprocess, we could record the location at build time after we detect the gap executable. That would introduce a new GAP_FOO variable into the config file, but we wouldn't need to pass it to gap/libgap at startup (they already know their own root dir), and most importantly, distro maintainers wouldn't need to mess with it.

@tobiasdiez
Copy link
Owner

So if I understand you correctly, your proposal would be to keep GAP_ROOT_PATHS, but only for the workspace feature? For the implementation, we currently detect this path during build time in meson (see the changed meson file here in this PR).

(This PR was triggered by sagemath#40327 (comment) where you suggested that one might be able to completely remove this variable).

@orlitzky
Copy link
Collaborator

First, we can stop passing the GAP_ROOT_PATHS to gap/libgap. This is what will make it impossible to use sage's gap_packages with the system gap, but it simplifies a lot of things.

Right now we are using the complicated

$ gap -r -q --bare --nointeract -c 'Display(JoinStringsWithSeparator(GAPInfo.RootPaths,";"));'

to get GAP_ROOT_PATHS. In addition to /usr/lib/gap, this also gets you $HOME/.gap and /usr/share/gap. The former is nice and the latter is necessary because we are overriding the default gap root paths with -l. Specifically:

$ gap -l $(gap --print-gaproot)
gap: hmm, I cannot find 'lib/init.g' maybe use option '-l <gaproot>'?

But if you don't pass -l, the default list of gap roots will be used, and it will include all of:

  1. /usr/lib/gap
  2. /usr/share/gap
  3. $HOME/.gap

This becomes relevant because, in the workspace code, we are only using two of those:

  1. /usr/lib/gap (can be obtained from gap --print-gaproot)
  2. $HOME/.gap (does not depend on anything)

In gap_workspace_file() we use /usr/lib/gap to find sysinfo.gap, and in timestamp() we use both of them to determine if any packages have been (un)installed.

So I think we have three options after removing -l $GAP_ROOT_PATHS and hard-coding $HOME/.gap.

  1. Use gap --print-gaproot on the fly to find /usr/lib/gap
  2. Use pkg-config --variable=libdir libgap on the fly to find /usr/lib/gap (this is a lot faster)
  3. Determine /usr/lib/gap at build time with either one of those, and then store it in GAP_ROOT_PATHS.

Option 3 is probably the fastest, but using pkg-config is also very fast, and has the benefit of eliminating the variable entirely.

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.

3 participants