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

Add Maximum Independent Set library #8337

Merged
merged 16 commits into from
Mar 22, 2024

Conversation

claud10cv
Copy link
Contributor

Tarballs for MaximumIndependentSet (originally in C++)

@imciner2
Copy link
Member

The upstream makefile seems to have -lcxxwrap_julia on the link line for the dynamic library, I don't see that on the command line for the Yggdrasil build you did (and you also don't include the libcxxwrap_julia_jll as a dependency). Are you packaging the most recent version of the library, or purposefully holding it back?

products,
dependencies,
julia_compat = "1.9";
preferred_gcc_version=v"11")
Copy link
Member

Choose a reason for hiding this comment

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

Is GCC 11 really needed? It looks like C++17 is used, which I think is available starting in GCC 7. In general, it is better to use as old a GCC version to build as possible because that ensures the best compatibility for the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to https://gcc.gnu.org/projects/cxx-status.html#cxx17 full support for c++17 is introduced as for g++-9. I will modify the line accordingly

Comment on lines 15 to 18
platforms = supported_platforms()
filter!(t -> os(t) in ["linux"], platforms)
filter!(t -> arch(t) == "x86_64", platforms)
filter!(t -> isnothing(libc(t)) || libc(t) == "glibc", platforms)
Copy link
Member

Choose a reason for hiding this comment

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

This is a slightly awkward way of just specifying the one platform, but it works. Out of curiosity though, is there a reason to not build on the other platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The makefile is a hard-coded, hand-written one. It only works on linux for now. I will work on creating a cmake script to enable cross-compile later this week

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am testing a cmake build and will see if I can get to coompile on more platforms

@imciner2 imciner2 changed the title Create build_tarballs.jl Add Maximum Independent Set library Mar 21, 2024
@claud10cv claud10cv marked this pull request as draft March 21, 2024 15:40
@claud10cv
Copy link
Contributor Author

The upstream makefile seems to have -lcxxwrap_julia on the link line for the dynamic library, I don't see that on the command line for the Yggdrasil build you did (and you also don't include the libcxxwrap_julia_jll as a dependency). Are you packaging the most recent version of the library, or purposefully holding it back?

Originally I had considered adding the wrapper within the C++ lib but ultimately decided to decouple both things, so I removed all references to libcxxwrap. I will create a separate wrapper package

@claud10cv claud10cv marked this pull request as ready for review March 21, 2024 17:44
@claud10cv claud10cv marked this pull request as draft March 21, 2024 18:13
@claud10cv claud10cv marked this pull request as ready for review March 21, 2024 20:11
@claud10cv claud10cv marked this pull request as draft March 21, 2024 20:11
@claud10cv claud10cv marked this pull request as ready for review March 21, 2024 20:31
Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

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

If you link to libjulia you really must follow what other packages do here, see for example: https://github.com/JuliaPackaging/Yggdrasil/blob/5dd8bee7317d0c5d0f797ae2328a069890eda5c4/L/libsingular_julia/build_tarballs.jl, since julia itself breaks the ABI in each minor release.

Highlights are

# These are the platforms we will build for by default, unless further
# platforms are passed in on the command line
include("../../L/libjulia/common.jl")
platforms = vcat(libjulia_platforms.(julia_versions)...)
filter!(!Sys.iswindows, platforms) # Singular does not support Windows
platforms = expand_cxxstring_abis(platforms)

(without the exclusion of Windows, if it isn't relevant for your case)
BuildDependency(PackageSpec(;name="libjulia_jll", version=v"1.10.9")),

Dependency("libcxxwrap_julia_jll"; compat = "~0.11.2"),

Comment on lines 24 to 27
platforms = supported_platforms(;
exclude=[Platform("i686", "linux"; libc = "musl")
]
)
Copy link
Member

Choose a reason for hiding this comment

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

https://buildkite.com/julialang/yggdrasil/builds/9118#018e62ae-0e18-4c32-bb7b-99ef8051ce83/663-878

┌ Warning: /cache/build/yggy-amdci7-15/julialang/yggdrasil/M/MaximumIndependentSet/build/x86_64-linux-gnu/clphCAVm/x86_64-linux-gnu-libgfortran5-cxx11/destdir/lib/libmis.so contains std::string values!  This causes incompatibilities across the GCC 4/5 version boundary.  To remedy this, you must build a tarball for both GCC 4 and GCC 5.  To do this, immediately after your `platforms` definition in your `build_tarballs.jl` file, add the line:
│ 
│     platforms = expand_cxxstring_abis(platforms)
└ @ BinaryBuilder.Auditor /cache/julia-buildkite-plugin/depots/e2fd9734-29d8-45cd-b0eb-59f7104f3131/packages/BinaryBuilder/L8tBh/src/auditor/compiler_abi.jl:247
Suggested change
platforms = supported_platforms(;
exclude=[Platform("i686", "linux"; libc = "musl")
]
)
platforms = supported_platforms(; exclude=p->libc(p)=="musl" && arch(p)=="i686")
platforms = expand_cxxstring_abis(platforms)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I believe I implemented all the changes suggested (except for the last one that clashes with on of the other suggestions above)

@claud10cv claud10cv marked this pull request as draft March 21, 2024 22:35
@giordano
Copy link
Member

giordano commented Mar 21, 2024

I forgot you need also

# See https://github.com/JuliaLang/Pkg.jl/issues/2942
# Once this Pkg issue is resolved, this must be removed
uuid = Base.UUID("a83860b7-747b-57cf-bf1f-3e79990d037f")
delete!(Pkg.Types.get_last_stdlibs(v"1.6.3"), uuid)

@claud10cv
Copy link
Contributor Author

I forgot you need also

# See https://github.com/JuliaLang/Pkg.jl/issues/2942
# Once this Pkg issue is resolved, this must be removed
uuid = Base.UUID("a83860b7-747b-57cf-bf1f-3e79990d037f")
delete!(Pkg.Types.get_last_stdlibs(v"1.6.3"), uuid)

Yep indeed without that the compiler complains about that UUID. Thank you for pointing out to the solution (or more like a workaround apparently)

@claud10cv claud10cv marked this pull request as ready for review March 21, 2024 23:08
M/MaximumIndependentSet/build_tarballs.jl Outdated Show resolved Hide resolved
M/MaximumIndependentSet/build_tarballs.jl Outdated Show resolved Hide resolved
@giordano giordano enabled auto-merge (squash) March 21, 2024 23:54
@giordano giordano merged commit ad89221 into JuliaPackaging:master Mar 22, 2024
17 checks passed
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