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

Convert HelFEM to use new libcxxwrap / libjulia #2320

Merged
merged 1 commit into from
Jan 9, 2021

Conversation

fingolfin
Copy link
Member

Make some progress towards issue #2160

Should not be merged before @mortenpi had a chance to review and comment. Also, there are some questions to discuss:

  1. Which version to use for this JLL? It really shouldn't be 0.0.3, as the list of dependencies changes, and that requires a new version.
  2. Which Julia versions should this JLL be made compatible with? Right now, this is built for Julia 1.5.3 or later, and also only usable in Julia 1.5. But we could make versions supporting 1.4 or even 1.3 if that is desirable. But it's a bit extra work, so I first want to get this version working, and also hear from @mortenpi what his preferences are.
  3. The two first points are actually somewhat related: in order to support multiple Julia versions, the trick we used in other JLLs is to use the patch level of the version string to indicate the Julia version; that would suggest to reversion this package to e.g. 0.3.X where X is equal to the one in the Julia version 1.X. However, that would mean packages using HelFEM_jll may need to be updated. Then again, I found no such package in the general registry?

]
include("../../L/libjulia/common.jl")
platforms = libjulia_platforms(julia_version)
platforms = expand_cxxstring_abis(platforms)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: This PR also expands the list of supported platforms considerably.

-DUSE_OPENMP=OFF \
-DHELFEM_BINARIES=OFF -DHELFEM_FIND_DEPS=ON \
-B build/ -S .
Copy link
Member Author

Choose a reason for hiding this comment

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

I took the liberty of sorting these arguments alphabetically and generally aligning them with other JLLs, in the hope that this will help a bit with future maintenance.

@mortenpi
Copy link
Contributor

mortenpi commented Jan 5, 2021

First, thanks a lot @fingolfin for all your work on this!

Now, I was trying to test this locally, just to make sure it works, but I'm running into some weird version conflict with OpenBLAS:

ERROR: LoadError: Unsatisfiable requirements detected for package OpenBLAS_jll [4536629a]:
 OpenBLAS_jll [4536629a] log:
 ├─possible versions are: 0.3.10 or uninstalled
 └─restricted to versions 0.3.9 by an explicit requirement — no versions left

Changing the compat to 0.3.10 doesn't help either:

ERROR: LoadError: Unsatisfiable requirements detected for package OpenBLAS_jll [4536629a]:
 OpenBLAS_jll [4536629a] log:
 ├─possible versions are: 0.3.10 or uninstalled
 ├─restricted to versions 0.3.10 by an explicit requirement, leaving only versions 0.3.10
 └─found to have no compatible versions left with libjulia_jll [5ad3ddd2]
   └─libjulia_jll [5ad3ddd2] log:
     ├─possible versions are: 1.3.1-1.5.3 or uninstalled
     └─restricted to versions 1.5.3 by an explicit requirement, leaving only versions 1.5.3

I wonder if it's because I'm running BB with Julia 1.6 and 1.7? I do have a non-standard setup. Could we re-run the Yggdrasil builds to make sure that no funky version problem has been introduced to the registry?

That said, it looks like the binaries were built successfully on Yggdrasil and I presume were fine. So I don't mind if we just merge this (after updating the version number) and I'll just test the registered binaries, and try to get my local workflow working later.

About the questions in the OP:

  • Personally I don't mind if it's 1.5+ only, I'm pretty much the only user (and the only consumer of these binaries is this currently unregistered package). Adding support for older Julia versions can be done as the need arises.
  • But I guess it might make sense to prepare for that eventuality, so how about setting the version to 0.1.5? The current version number has no particular meaning.

Regarding the changes to the ordering of the cmake commands etc -- LGTM!

@fingolfin
Copy link
Member Author

@mortenpi Yggdrasil runs on development version of BinaryBuilder and BinaryBuilderbase only for now -- and unfortunately may also need an older snapshot of the Julia master, namely from before the "fake JLL" code was merged there -- any commit before 2eccd8e086ba1b08dd8b4d04e86b548fcf8ec5e5 on the Julia repository should do it.

The reason for this is that we need to install specific version of certain JLLs (which include OpenBLAS_jll) in order build specific versions of libjulia_jll ; but since the latest Julia master branch now ships those JLLs (including OpenBLAS_jll) as standard libs, we cannot install different / older versions of them.

This is a serious problem going forward; I am not sure if anybody has a plan how we'll address this. I am kinda hoping @staticfloat has one, or perhaps @giordano ?

@mortenpi
Copy link
Contributor

mortenpi commented Jan 6, 2021

Thanks for the tip! I compiled Julia off a178ac, which is the one just before the fake JLL patch, and the JLL compilation went without a hitch. As far as I can tell, the binaries work as expected, so this could be merged as is.

@fingolfin fingolfin mentioned this pull request Jan 7, 2021
@fingolfin fingolfin marked this pull request as ready for review January 8, 2021 23:59
@giordano giordano closed this Jan 9, 2021
@giordano giordano reopened this Jan 9, 2021
@giordano giordano merged commit d4e2e8a into JuliaPackaging:master Jan 9, 2021
@fingolfin fingolfin deleted the mh/HelFEM branch January 9, 2021 02:00
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.

3 participants