Skip to content

AlgoimDiff to point to updated c++ repo#14043

Open
jehicken wants to merge 5 commits into
JuliaPackaging:masterfrom
jehicken:master
Open

AlgoimDiff to point to updated c++ repo#14043
jehicken wants to merge 5 commits into
JuliaPackaging:masterfrom
jehicken:master

Conversation

@jehicken

Copy link
Copy Markdown
Contributor

This is a simple update to the commit in the sources variable of the build_tarballs.jl file. I also updated the version.

Point toward an updated version of the c++ source
Pointed to updated c++ commit.
I have had to severely limit the platforms supported, because the build system cannot find the jlcxx.hpp header on many systems.
)
platforms = expand_cxxstring_abis(platforms)
filtered_platforms = filter(supported_platforms()) do p
Sys.islinux(p) && arch(p) == "x86_64"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you are quite drastically reducing the number of platforms here. Is that intended? For example x86_64-apple-darwin used to be supported but is no longer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it was intentional. For many platforms, the current script in build_tarballs.jl is unable to find the directories where the CxxWrap headers are (e.g.jlcxx.hpp). I am not sure what changed. I spent many hours looking through other packages that use CxxWrap to find workarounds without success.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @lgoettgens, do I need to resolve this converstation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not happy with this hard of a platform restriction in a minor version bump, which is why I haven't approved this. If you find another yggdrasil maintainer that merges this PR as is, that is an ok solution for me.
Otherwise, you could enable more platforms again (ideally all that should in theory be supported) and have people here help you with debugging.

One thing that I see immediately is that since you rely on libjulia and libcxxwrap, you need to do a separate build per julia minor release. Otherwise, your package will probably not work properly due to ABI differences between different julia versions. Please have a look at other recipes depending on libcxxwrap (e.g. Geant4_julia) for a blueprint on how to do that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes. And it's not clear to me why the header files shouldn't be found.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@lgoettgens and @giordano: Ok, I will take another crack at this if and when I find time.

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