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

Merge libsingular-julia into this report and set up a system where it automatically is used to replace the JLL if appropriate #600

Closed
fingolfin opened this issue Nov 3, 2022 · 5 comments

Comments

@fingolfin
Copy link
Member

I propose that we put the code (and history) https://github.com/oscar-system/libsingular-julia back into this repository; but with a slight twist that I'll explain shortly.

The motivation is that often fixes require changes both in Singular.jl and in libsingular-julia. This is more convenient if they can be put into a single PR. Of course in practice to deploy these fixes we then still need to update the libsingular_julia_jll. And to test these changes one has to follow a carefully choreographed dance with overrides and whatnot.

But instead, I suggest to adapt a technique I am successfully using for GAP.jl. It works something like this:

  1. in the JLL (for the GAP the relevant JLL is GAP_pkg_juliainterface_jll) I track which source code was used to produce the JLL. That could be a git tree hash of the relevant source directory
    • in GAP.jl, to simplify the code building the JLL, I followed an even simpler approach and copied all the *.c and *.h files into the JLL and then I later compute a tree hash of them in Julia. I should change that and somehow compute the tree hash in the code building the JLL, back then I just used the quickest way I saw to get what I want (i.e. I was lazy)
    • do not just use the git SHA1 of a commit: often multiple commits in the main repository will contain the same C/C++ code, no need to distinguish them
  2. now when loading Singular.jl, after loading libsingular_julia_jll, check if it references the same source code as is bundled, by comparing the git tree hash or whatever. If they match, just use the JLL. But if they do not match... compile the source code (i.e., invoke cmake) and load that. Here is the relevant function for that from GAP.jl:
function locate_JuliaInterface_so(sysinfo::Dict{String, String})
    # compare the C sources used to build GAP_pkg_juliainterface_jll with bundled copies
    # by comparing tree hashes
    jll = GAP_pkg_juliainterface_jll.find_artifact_dir()
    jll_hash = Pkg.GitTools.tree_hash(joinpath(jll, "src"))
    bundled = joinpath(@__DIR__, "..", "pkg", "JuliaInterface")
    bundled_hash = Pkg.GitTools.tree_hash(joinpath(bundled, "src"))
    if jll_hash == bundled_hash
        # if the tree hashes match then we can use JuliaInterface.so from the JLL
        @debug "Use JuliaInterface.so from GAP_pkg_juliainterface_jll"
        path = joinpath(jll, "lib", "gap")
    else
        # tree hashes differ: we must compile the bundled sources
        path = build_JuliaInterface(sysinfo)
        @debug "Use JuliaInterface.so from $(path)"
    end
    return joinpath(path, "JuliaInterface.so")
end

This has simplified my life working on the C code in GAP.jl drastically. I now edit the C/C++ code with impunity in tandem with the Julia code. Once this works on my computer, I can submit it as a PR, and CI passes. Once all is good, I can merge the PR and only then do I update the JLL.

The one drawback of my current setup has is that I should add a CI test that prevents me from making a release if the JLL is outdated. (E.g. it could be a regular CI test that fails even on the PR, but I could ignore it on PRs. Or the JLL could be updated while the PR is approve but not yet merged, whatever, these are minor details).

All in all this is not hard to pull off but IMHO would be a great benefit for Singular.jl. At least for me this is much easier to work than dealing with JLL overrides; it is 100% automatic. No matter how good our override scripts are, they still require more manual work and offer more chances for me to screw up using them correctly...

(Perhaps it would also be useful for Polymake.jl, @benlorenz ?)

@tthsqe12
Copy link
Contributor

tthsqe12 commented Nov 4, 2022

This is my workflow. Can you comment on exactly what would change in the loop?

1. Make sure the override is set
loop
  2. edit the C code
  3. run `julia build.jl` on the C code
  3. edit the julia code
  4. if no source code changed in Step 3, just touch Singular.jl so that Step 6 re-precompiles
  5. restart julia
  6. do `using Singular` test changes
end loop

@fingolfin
Copy link
Member Author

Assuming no changes in Singular_jll (those would still need an override and extra work):

loop
  1. edit the C code
  2. edit the julia code
  3. restart julia
  4. do `using Singular` test changes
end loop

@fingolfin
Copy link
Member Author

(It would also make it easy to track coverage of the C++ code, see issue #224, but that's a minor side concern)

@fingolfin
Copy link
Member Author

Reminder to myself: to get the tree hash e.g. the deps/src dir:

  • using git on a repo: git ls-tree --object-only HEAD deps/src
  • using Julia for some path: Base.SHA1(Pkg.GitTools.tree_hash("deps/src"))

@fingolfin
Copy link
Member Author

Resolve by #639

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

No branches or pull requests

2 participants