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

Make library relocatable #362

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

Conversation

tmmsartor
Copy link

Me and @jgillis are developing a C interface for MadNLP , so that it can be bundled in a "standalone" shared library and used from non-Julia tools and frameworks.
We are using PackageCompiler.jl to create the library, but we encountered some issues with relocatability, namely having the Julia depot path of the building machine hard-coded in some places.

Those commits seems to fix the problem in our build, even if I am not 100% sure this is the best way to solve the problem, given I am relatively new to Julia.

Would you be interested in making MadNLP.jl relocatable?

@frapac
Copy link
Collaborator

frapac commented Jul 26, 2024

Hi @tmmsartor , we found your project on github a week ago and are very curious about it.

We would be in to make MadNLP relocatable, if that could make your life easier. Looking at your PR, it looks like you have identified two issues:

  1. we parse the toml file to get the current version of MadNLP
  2. the MUMPS's interface is using ccall instead of @call
    Is that right?

Apparently RelocatableFolders.jl is needed only for 1. Let me check if we can find an alternative to using this new dependency.

src/MadNLP.jl Outdated
@@ -15,7 +16,9 @@ export MadNLPSolver, MadNLPOptions, UmfpackSolver, LDLSolver, CHOLMODSolver, Lap
import LDLFactorizations

# Version info
version() = parsefile(joinpath(@__DIR__,"..","Project.toml"))["version"]
# version() = parsefile(joinpath(@__DIR__,"..","Project.toml"))["version"]
const PROJECT_TOML = @path joinpath(@__DIR__,"..","Project.toml")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the following work with PackageCompiler.jl?

version() = packageversion(@__MODULE__)

Using that we don't have to parse the toml file and won't have to depend on RelocatableFolders.jl.

This requires Julia >= 1.9

Copy link
Author

Choose a reason for hiding this comment

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

I can try but I don't find this packageversion method is custom or part of some Package?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My apologies, I meant pkgversion instead of packageversion. The function is part of Base.

@tmmsartor
Copy link
Author

Yes I think those are the two issue we found.
I also think first issue can be solved without depending on RelocatableFolders.jl

Regarding the second issue I am not is the problem is @ccall vs ccall or the fact the artifacts lib path is appearing in an eval block.
I notice that only for libmumps there was an hard-coded full path in the library, while other external libraries like cudss or metis were working fine, so I try to adapt the calling style to those cases.

@frapac
Copy link
Collaborator

frapac commented Jul 26, 2024

I think it's a good idea to remove the @eval block, and I am in to replace it by @ccall.

If the tests pass and you confirm that you can use PackageCompiler.jl, we can merge this PR.

Copy link
Collaborator

@frapac frapac left a comment

Choose a reason for hiding this comment

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

LGTM

@tmmsartor
Copy link
Author

I was not sure if there was a reason why in MadNLP is defined the lambda/method version() while in MadNLPMumps there is the variable version, to be safe I keep this distinction in the relocatability updates.
Also pkgversion() return VersionNumber while MadNLP tests seems to expect a string for version()/version so I added that conversion.
I was wondering if there is Julia standard or suggested way to provide this version information.

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