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 CI job against GAP master #995

Merged
merged 18 commits into from
May 31, 2024
Merged

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented May 22, 2024

Resolves the GAP.jl part of #994

This now seems to work.

This currently contains #999, and should be rebased once merged.

Copy link

codecov bot commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.75%. Comparing base (597b2f3) to head (d61e12a).

Current head d61e12a differs from pull request most recent head f9756a4

Please upload reports for the commit f9756a4 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #995   +/-   ##
=======================================
  Coverage   75.75%   75.75%           
=======================================
  Files          51       51           
  Lines        4190     4191    +1     
=======================================
+ Hits         3174     3175    +1     
  Misses       1016     1016           
Files Coverage Δ
src/setup.jl 79.64% <100.00%> (+0.18%) ⬆️

@lgoettgens
Copy link
Member Author

The helpsystem tests fail. @fingolfin do you have any idea whats going on there?

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 making a start on this!

We need some addition tweaks. For example, we should force JuliaInterface being rebuilt. It could be as simple as doing echo >> pkg/JuliaInterface/src/JuliaInterface.c which modifies that file in a trivial

The test failures are there because etc/setup_override_dir.jl does not install the GAP documentation. So we either skip those tests. Or we need to build and install the GAP docs. That could be done by doing make doc (but that requires latex, and runs quite a bit; and it also needs GAPDoc.... so one would have to install at least that...) and then replace

run(`make install-bin install-headers install-libgap install-sysinfo install-gaproot`)

by

run(`make install`)

A much simpler and faster solution might be to take the docs from the GAP_lib_jll (e.g. adding a symlink to the doc dir in there into the right subdir of /tmp/gap_jll_override ... might be /tmp/gap_jll_override/share/gap/doc but I'd have to look it up).

BTW do you have any idea what might cause these warnings?

#I  The Julia package 'Downloads' cannot be loaded.

.github/workflows/gap.yml Outdated Show resolved Hide resolved
.github/workflows/gap.yml Outdated Show resolved Hide resolved
etc/setup_override_dir.jl Show resolved Hide resolved
.github/workflows/gap.yml Outdated Show resolved Hide resolved
@lgoettgens lgoettgens force-pushed the lg/GAP-CI branch 4 times, most recently from 20244e1 to af9f651 Compare May 25, 2024 15:51
Copy link
Member Author

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

it now works more or less for ubuntu. for macos there is some issue with JuliaInterface that I don't know how to handle

Pkg.develop(path=dirname(dirname(@__FILE__)))
Pkg.add(["GAP_jll", "GAP_lib_jll"])
Copy link
Member Author

Choose a reason for hiding this comment

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

This changed order of additions makes a little bit faster to precompile

pkgid = Base.identify_package("$(pkgname)_jll")
uuid = string(pkgid.uuid)
# does not work with julia 1.12-dev
# pkguuid = string(pkgid.uuid)
Copy link
Member Author

Choose a reason for hiding this comment

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

This threw some (seemingly random) segfaults with nightly. But only if running from the console, not if copying everything to the reply.
Thus I took the easy solution and added the UUID to the arguments instead

Copy link
Member

Choose a reason for hiding this comment

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

Just tried with Julia master and when running this code I actually got

ERROR: LoadError: type Nothing has no field uuid

which would suggest pkgid === nothing, but then also the touch below would be borked?

Copy link
Member Author

Choose a reason for hiding this comment

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

if you print the pkgid, this succeeds (and prints something non-trivial), but i then get a segfault instead of a LoadError when accessing pkgid.uuid

Copy link
Member

Choose a reason for hiding this comment

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

Indeed: printing the pkgid makes it proceed -- that seems like a big regression in Julia, printing a value shouldn't change it form nothing to something, should it? In particular since this works in the REPL (without printing):

pkgid = Base.identify_package("GAP_jll"); pkgid.uuid

Perhaps we should open an issue?

Anyway, so I inserted the print and it proceeds, but I don't get a crash, I get this:

[ Info: Created temporary depot at /var/folders/d_/1zss2fnd6xdgclqnj8jj_27m0000gp/T/jl_7ABU3q
pkgname GAP has pkgid Base.PkgId(Base.UUID("5cd7a574-2c56-5be2-91dc-c8bc375b9ddf"), "GAP_jll")
pkgname GAP_lib has pkgid Base.PkgId(Base.UUID("de1ad85e-c930-5cd4-919d-ccd3fcafd1a3"), "GAP_lib_jll")
Precompiling GAP...
Info Given GAP was explicitly requested, output will be shown live
ERROR: LoadError: ArgumentError: Package GAP_jll [5cd7a574-2c56-5be2-91dc-c8bc375b9ddf] is required but does not seem to be installed:
 - Run `Pkg.instantiate()` to install all recorded dependencies.

which makes no sense, as we run Pkg.instantiate(). But I've inserted another Pkg.instantiate() invocation right after the add_jll_override calls.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for reporting it upstream, it seems we've hit an IMHO rather serious looking code generation bug in Julia.

For now we can keep this workaround, but let's remove it again when they fix their bug.

etc/run_with_override.jl Outdated Show resolved Hide resolved
etc/run_with_override.jl Show resolved Hide resolved
@fingolfin
Copy link
Member

For the macOS failures you basically need to insert sysinfo["GAC_LDFLAGS"] = "-bundle" into regenerate_gaproot(). This works around a bug in the GAP_jll build recipe and will be fixed in its next version.

@lgoettgens lgoettgens force-pushed the lg/GAP-CI branch 2 times, most recently from a5f3b8a to 2191928 Compare May 27, 2024 16:20
src/setup.jl Outdated Show resolved Hide resolved
src/setup.jl Outdated Show resolved Hide resolved
.github/workflows/gap.yml Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

This is shaping up quite well, thank you @lgoettgens

@lgoettgens
Copy link
Member Author

I now reduced the number of jobs to 6 ubuntu ([master, stable-4.13] x [1.6, 1, nightly]) and 2 macOS ([master, stable-4.13] x [1]).

@lgoettgens lgoettgens marked this pull request as ready for review May 28, 2024 08:25
etc/run_with_override.jl Outdated Show resolved Hide resolved
@fingolfin fingolfin merged commit fed22f9 into oscar-system:master May 31, 2024
21 checks passed
@lgoettgens lgoettgens deleted the lg/GAP-CI branch June 4, 2024 16:17
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.

2 participants