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

Replace Setup.regenerate_gaproot() by Setup.gaproot_for_building() #1162

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

lgoettgens
Copy link
Member

Implements the second bullet point of #1158 in the following way:

  • Add a new function gaproot_for_building() that creates a temporary directory just containing sysinfo.gap and gac (as they are currently in gaproot_mutable). This directory is cached for the duration of the current session, but each time we want to use it, we check that the content hasn't been deleted by some OS cronjob that cleans /tmp regularly.
  • This gaproot_for_building is injected both into the build process of JuliaInterface and the PackageManager right before calling InstallPackage or CompilePackage.
  • Remove all mentions of the gaproot_mutable that we had previously.

Resolves #840.

For testing the compilation of packages, we need a package with a kernel extension that has no jll (and will never get one). The prime candidate would be example. However, its TestPackageAvailability does not tell us anything about the build status of its kernel extension, so I don't see a way to tell if compiling it has actually worked. For the meantime, I disabled GAP_pkg_json_jll and use json for testing, but this should be changed before merging. @fingolfin do you have an idea how to do a proper test here?

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.46%. Comparing base (c33287b) to head (fb61078).

Files with missing lines Patch % Lines
src/packages.jl 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1162      +/-   ##
==========================================
+ Coverage   75.43%   75.46%   +0.03%     
==========================================
  Files          55       55              
  Lines        4709     4716       +7     
==========================================
+ Hits         3552     3559       +7     
  Misses       1157     1157              
Files with missing lines Coverage Δ
src/GAP.jl 86.88% <100.00%> (-0.22%) ⬇️
src/GAP_pkg.jl 100.00% <ø> (+3.33%) ⬆️
src/setup.jl 88.49% <100.00%> (+0.75%) ⬆️
src/packages.jl 49.77% <50.00%> (+<0.01%) ⬆️

@lgoettgens
Copy link
Member Author

Uuh, Oscar also wants to put things in a gap root, notably https://github.com/oscar-system/Oscar.jl/blob/7c2d569c224a462f05d0e2d50ebf0da024b0dc3e/src/GAP/utils.jl#L2 which is called from the initialization code of the perfect groups library.
I think the cleanest solution for this would be to instead let Oscar create its own gap root in a scratch space with these files and then call GAP.Globals.ExtendRootPaths during __init__ to hook this into GAP. I'll try to get this working in a matching branch tomorrow, so we can let CI test it together.

In any case, this PR will need a breaking bump in GAP.jl's Version number.

@fingolfin
Copy link
Member

As for testing we could use a package that is guaranteed to never be shipped in the distro. https://github.com/gap-packages/RegisterPackageTNUMDemo might be perfect candidate. I just made a fresh release of it.

@fingolfin
Copy link
Member

Yeah it would be good to get rid of __init_extraperfect. In this case I actually think there is a very simple solution on the Oscar.jl side: Right now we do

function __init_extraperfect()
  for i in [27, 33]
    _write_gap_file(
      "grp/perf$(i).grp",
      "Read(JuliaToGAP(IsString, Oscar._path_extraperfect($(i))));\n",
    )
  end
end

(Hm, shouldn't that be using Oscar_jl. now?)

Instead of creating these files dynamically, we could just put them all permanently into gap/grp/ in the Oscar.jl repository, and add that dir as a root path to GAP. (If we then move OscarInterface into a new pkg subdir we then could also get rid of the GAP.Globals.SetPackagePath call for it in src/Oscar.jl).

Having such a GAP root in Oscar might be useful to other things, too (e.g. we can use it to overload specific GAP library files.

Anyway, just a thought. Using a Julia scratch space or an OS temp dir created via mktemp both would also work just fine, I'd be happy with any of those solutions.

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 for working on this!

const sysinfo = Setup.regenerate_gaproot()

const GAP_VERSION = VersionNumber(sysinfo["GAP_VERSION"])
const GAP_VERSION = VersionNumber(Setup.read_sysinfo_gap(joinpath(GAP_jll.find_artifact_dir(), "lib", "gap", "sysinfo.gap"))["GAP_VERSION"])
Copy link
Member

Choose a reason for hiding this comment

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

I actually have written code in the past that used GAP.sysinfo. From that POV it would be nice to at least retain it, e.g.

Suggested change
const GAP_VERSION = VersionNumber(Setup.read_sysinfo_gap(joinpath(GAP_jll.find_artifact_dir(), "lib", "gap", "sysinfo.gap"))["GAP_VERSION"])
const sysinfo = Setup.read_sysinfo_gap(joinpath(GAP_jll.find_artifact_dir(), "lib", "gap", "sysinfo.gap"))
const GAP_VERSION = VersionNumber(sysinfo["GAP_VERSION"])

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference would be that GAP.sysinfo used to be the sysinfo with updated fields that was created during startup, but with this change it would instead be the sysinfo that is bundled with GAP_jll.

Comment on lines +212 to 214
sysinfo = read_sysinfo_gap(joinpath(gaproot, "sysinfo.gap"))

return normpath(joinpath(jipath, "bin", sysinfo["GAParch"]))
Copy link
Member

Choose a reason for hiding this comment

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

If we add the global sysinfo back, then this could also be

Suggested change
sysinfo = read_sysinfo_gap(joinpath(gaproot, "sysinfo.gap"))
return normpath(joinpath(jipath, "bin", sysinfo["GAParch"]))
return normpath(joinpath(jipath, "bin", GAP.sysinfo["GAParch"]))

assuming that something like import ...GAP: GAP is added at the top of this file

@lgoettgens
Copy link
Member Author

As for testing we could use a package that is guaranteed to never be shipped in the distro. gap-packages/RegisterPackageTNUMDemo might be perfect candidate. I just made a fresh release of it.

I am not sure this is the right candidate, because it does not check if its kernel extension is indeed available in TestPackageAvailability (https://github.com/gap-packages/RegisterPackageTNUMDemo/blob/af1401c5bdc10872ccb6169ddf0f7dae99bc33be/PackageInfo.g#L94-L96). Or is this something that should be changed in RegisterPackageTNUMDemo since it would be helpful to have it there as well?

@lgoettgens
Copy link
Member Author

Instead of creating these files dynamically, we could just put them all permanently into gap/grp/ in the Oscar.jl repository, and add that dir as a root path to GAP. (If we then move OscarInterface into a new pkg subdir we then could also get rid of the GAP.Globals.SetPackagePath call for it in src/Oscar.jl).

Having such a GAP root in Oscar might be useful to other things, too (e.g. we can use it to overload specific GAP library files.

I really like that idea. Didn't know that this would work, but I'll give it a go.

Anyway, just a thought. Using a Julia scratch space or an OS temp dir created via mktemp both would also work just fine, I'd be happy with any of those solutions.

If we would ever want to change any files in there, and people use multiple different Oscar versions on the same system, we get basically to the same discussion that we already had in #1079. I.e. how to parameterize the scratch keys, and when to clean up and how to prevent the OS from cleaning up too early.

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.

Factor out check for C/C++ compiler (so it doesn't slow down our startup each time)
2 participants