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

Custom installations of Gmsh via Preferences.jl #31

Closed
wants to merge 18 commits into from

Conversation

fverdugo
Copy link

Hi there!

This PR allows users to use their custom installation of Gmsh. The new logic is implemented using Preferences.jl

Two new functions are added

  • Gmsh.use_system_gmsh. Calling this function will configure Gmsh to use a system installation. It will look for an installation in the system (LD_LIBRARY_PATH), or the user can provide the path to the installation. Changes will be effective in the next Julia session.
  • Gmsh.use_gmsh_jll. Calling this function will configure Gmsh to use the binary provided by gmsh_jll (e.g., to revert a previous call to Gmsh.use_system_gmsh). Changes will be effective in the next Julia session.

The CI is updated to test Gmsh.jl with a custom installation of Gmsh.

@fverdugo
Copy link
Author

cc @ahojukka5 @KristofferC

@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 9.75610% with 37 lines in your changes are missing coverage. Please review.

Project coverage is 29.62%. Comparing base (b521836) to head (a469cde).

Files Patch % Lines
src/Gmsh.jl 9.75% 37 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #31       +/-   ##
===========================================
- Coverage   92.30%   29.62%   -62.68%     
===========================================
  Files           1        1               
  Lines          13       54       +41     
===========================================
+ Hits           12       16        +4     
- Misses          1       38       +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fredrikekre
Copy link
Collaborator

Is there a reason not to use the regular override mechanism (https://pkgdocs.julialang.org/v1/artifacts/#Overriding-artifact-locations)?

@fverdugo
Copy link
Author

Hi @fredrikekre

IMHO, understanding how artifacts work and setting up an override looks way more complicated than reading the documentation of a function, calling the function, and restarting Julia (proposed solution).

In addition, the Override.toml seems to affect all the packages in the current depot:

To enable this, Pkg supports a per-depot Overrides.toml file placed within the artifacts depot directory (e.g. ~/.julia/artifacts/Overrides.toml for the default user depot) that can override the location of an artifact either by content-hash or by package UUID and bound artifact name.

Is it possible to have project A using gmsh_jll and project B using a system installed gmsh? I would say that this is not possible with overrides, but it is definitively possible with preferences.

@ahojukka5
Copy link
Member

Should this be merged?

@@ -1,10 +1,141 @@
module Gmsh

import gmsh_jll
include(gmsh_jll.gmsh_api)
@static if VERSION >= v"1.6"
Copy link
Collaborator

@fredrikekre fredrikekre Jun 14, 2024

Choose a reason for hiding this comment

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

You can remove these version checks and set julia compat to 1.6 instead.

@KristofferC
Copy link

KristofferC commented Jun 14, 2024

I think the Overrides.toml is pretty much deprecated. However, using preferences I think jlls already have support for this, see e.g. https://docs.binarybuilder.org/stable/jll/#Overriding-specific-products.

I don't think bespoke code should be added to binary wrappers when there is a generic solution to do exactly this. If that is faulting for some reason it should be improved.

@fverdugo
Copy link
Author

I think the Overrides.toml is pretty much deprecated. However, using preferences I think jlls already have support for this, see e.g. https://docs.binarybuilder.org/stable/jll/#Overriding-specific-products.

This looks promising. Do you have a concrete example showing how this would apply to Gmsh.jl?

@KristofferC
Copy link

Something like this perhaps (I haven't used it very much myself):

julia> using gmsh_jll

julia> gmsh_jll.gmsh_path
"/home/kc/.julia/artifacts/65888a08d8ab9e3bf5e28f26eabd2731f2a95ff3/bin/gmsh"

julia> using Preferences

julia> set_preferences!(
           "LocalPreferences.toml",
           "gmsh_jll",
           "libgmsh_path" => "/lib/x86_64-linux-gnu/libgmsh.so.4.8"
       )

# restart julia

julia> using gmsh_jll
Precompiling gmsh_jll
  1 dependency successfully precompiled in 1 seconds. 55 already precompiled.
ERROR: InitError: could not load library "/lib/x86_64-linux-gnu/libgmsh.so.4.8"
/lib/x86_64-linux-gnu/libgmsh.so.4.8: undefined symbol: _ZN24BRepBuilderAPI_MakeShape5BuildEv
Stacktrace:
  [1] dlopen(s::String, flags::UInt32; throw_error::Bool)
    @ Base.Libc.Libdl ./libdl.jl:117
  [2] dlopen(s::String, flags::UInt32)
    @ Base.Libc.Libdl ./libdl.jl:116

@fredrikekre
Copy link
Collaborator

fredrikekre commented Jun 14, 2024

This works:

julia> using Gmsh

julia> Gmsh.gmsh.GMSH_API_VERSION
"4.13.0"

julia> using Preferences

julia> set_preferences!(
           "LocalPreferences.toml",
           "gmsh_jll",
           "libgmsh_path" => abspath("gmsh-4.12.2-Linux64-sdk/lib/libgmsh.so"),
           "gmsh_api_path" => abspath("gmsh-4.12.2-Linux64-sdk/lib/gmsh.jl"),
       )

<Restart Julia>

julia> using Gmsh

julia> Gmsh.gmsh.GMSH_API_VERSION
"4.12.2"

but it requires adding Libdl as a dependency here in Gmsh because the downloaded gmsh.jl file from the sdk uses it, but the file is patched in the gmsh_jll (https://github.com/JuliaPackaging/Yggdrasil/blob/3e5207cb20f6438ff1ce6e502680b8c3593e5020/G/gmsh/build_tarballs.jl#L31-L36) to comment this out.

@fverdugo
Copy link
Author

What about hiding

julia> set_preferences!(
           "LocalPreferences.toml",
           "gmsh_jll",
           "libgmsh_path" => abspath("gmsh-4.12.2-Linux64-sdk/lib/libgmsh.so"),
           "gmsh_api_path" => abspath("gmsh-4.12.2-Linux64-sdk/lib/gmsh.jl"),
       )

in a function use_system_gmsh that automatically looks for gmsh in the system (like the one I implemented)?

@KristofferC
Copy link

that automatically looks for gmsh in the system (like the one I implemented)?

To me, that feels like it will be annoying to get correct on all systems. For example, the one in the PR doesn't find my libgmsh even though:

❯ ldconfig -p | grep libgmsh
        libgmsh.so.4.8 (libc6,x86-64) => /lib/x86_64-linux-gnu/libgmsh.so.4.8

There might also be multiple ones?

❯ ldconfig -p | grep libcurl
        libcurl.so.4 (libc6,x86-64) => /lib/x86_64-linux-gnu/libcurl.so.4
        libcurl.so.4 (libc6) => /lib/i386-linux-gnu/libcurl.so.4

Just feels easier to document:

  • How to change the gmsh library to a given path
  • Possible ways of finding out where a local gmsh lives.

@fverdugo
Copy link
Author

Just feels easier to document:

How to change the gmsh library to a given path
Possible ways of finding out where a local gmsh lives.

Agreed!

I would add:

And also test a custom installation in the CI tests.

@KristofferC
Copy link

And also test a custom installation in the CI tests.

Could always be added, but in some sense we are not really testing anything Gmsh-specific (Gmsh only knows about gmsh through gmsh_jll and JLLWrappers should have tests for Preferences path rewriting).

@fverdugo
Copy link
Author

I close the PR as the solution below by @fredrikekre is good enough. @fredrikekre @KristofferC Thanks for the feedback!

julia> using Gmsh

julia> Gmsh.gmsh.GMSH_API_VERSION
"4.13.0"

julia> using Preferences

julia> set_preferences!(
           "LocalPreferences.toml",
           "gmsh_jll",
           "libgmsh_path" => abspath("gmsh-4.12.2-Linux64-sdk/lib/libgmsh.so"),
           "gmsh_api_path" => abspath("gmsh-4.12.2-Linux64-sdk/lib/gmsh.jl"),
       )

<Restart Julia>

julia> using Gmsh

julia> Gmsh.gmsh.GMSH_API_VERSION
"4.12.2"

@fverdugo fverdugo closed this Jun 15, 2024
@fredrikekre
Copy link
Collaborator

I still think we need to add Libl as a dependency right? Would you make a PR? The CI test can also be kept I think, this repo is so low traffic anyway so no it isn't ao wasteful IMO. Perhaps also add this to the README?

@fverdugo
Copy link
Author

I still think we need to add Libl as a dependency right? Would you make a PR?

See PR #34

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.

5 participants