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

[Mousetrap] Add mousetrap #7461

Merged
merged 15 commits into from
Nov 1, 2023
Merged

Conversation

Clemapfel
Copy link
Contributor

@Clemapfel Clemapfel commented Oct 1, 2023

Supercedes #7347.

I neglected to expand for C++ string ABIs because my project requires gcc 11.0 or newer anyway, so there is no way to build with the old standard. Please correct me if this is improper.

@Clemapfel
Copy link
Contributor Author

Clemapfel commented Oct 1, 2023

I had to add the following line to the MESON_TARGET_TOOLCHAIN in the [binaries] section:

cmake='/usr/bin/cmake'

Otherwise it would not detect the cmake executable for me. Should I open an issue for this?

@Clemapfel Clemapfel marked this pull request as ready for review October 15, 2023 17:47
M/mousetrap/build_tarballs.jl Outdated Show resolved Hide resolved
M/mousetrap/build_tarballs.jl Outdated Show resolved Hide resolved
M/mousetrap/build_tarballs.jl Outdated Show resolved Hide resolved
@staticfloat staticfloat enabled auto-merge (squash) October 20, 2023 22:16
@staticfloat staticfloat disabled auto-merge October 20, 2023 22:16
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.

I neglected to expand for C++ string ABIs because my project requires gcc 11.0 or newer anyway, so there is no way to build with the old standard. Please correct me if this is improper.

Quoting from the docs

This ABI does not have to do with the C++ standard used by the source code, in fact you can build a C++03 library with the C++11 std::string ABI and a C++11 library with the C++03 std::string ABI. This is achieved by appropriately setting the _GLIBCXX_USE_CXX11_ABI macro.

Emphasis not mine.

]

# Build the tarballs, and possibly a `build.jl` as well.
build_tarballs(ARGS, name, version, sources, script, platforms, products, dependencies; julia_compat="1.7", preferred_gcc_version = v"12.1.0")
Copy link
Member

Choose a reason for hiding this comment

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

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 added two commits to expand for string ABIs and to use GCC 9, which is the oldest version supported. Thank you

@stemann
Copy link
Contributor

stemann commented Oct 22, 2023

Is support for Julia 1.7 and 1.8 needed? You are spending a lot of CI hours for building this...

@Clemapfel
Copy link
Contributor Author

Clemapfel commented Oct 22, 2023

I agree, but I was told by fingolfin to expand for different Julia versions. This and the string ABI expansion, both of which I didn't have initially but were requested during review, makes for a ridiculous amount of versions.

If I only use Julia 1.9, will that mean 1.8 and 1.7 users cannot use the jll package? I'd like to not introduce a version dependency when there isn't any reason for it other than for build time.

@stemann
Copy link
Contributor

stemann commented Oct 22, 2023

I agree, but I was told by fingolfin to expand for different Julia versions. This and the string ABI expansion, both of which I didn't have initially but were requested during review, makes for a ridiculous amount of versions.

If I only use Julia 1.9, will that mean 1.8 and 1.7 users cannot use the jll package? I'd like to not introduce a version dependency when there isn't any reason for it other than for build time.

Indeed.

Yes - that would drop support for 1.8 and 1.7 users.

@Clemapfel
Copy link
Contributor Author

Clemapfel commented Oct 22, 2023

I would be fine to change:

platforms = Platform[]
include("../../L/libjulia/common.jl")
for version in [v"1.7.0", v"1.8.2", v"1.9.0", v"1.10", v"1.11"]
    for platform in libjulia_platforms(version)
        if nbits(platform) != 32
            push!(platforms, platform)
        end
    end
end
platforms = expand_cxxstring_abis(platforms)

To

platforms = expand_cxxstring_abis(filter!(p -> nbits == 32, supported_platforms())

Since I'm not positive any of the Julia api changes between versions affect my project, but I would delegate that decision to you guys since I don't have any experience with deploying jlls.

@Clemapfel
Copy link
Contributor Author

Clemapfel commented Oct 22, 2023

I copied the versions expansion mostly from libcxxwrap_julia which also has that many versions (it actually has even more since it also allows for 32-bit architectures) and is already merged.

@ufechner7
Copy link

@Clemapfel Are you still waiting for further input, or do you think it can be merged?

@Clemapfel
Copy link
Contributor Author

I'm fine with it being merged as-is

@ufechner7
Copy link

@imciner2 Can you please approve?

@Clemapfel
Copy link
Contributor Author

Clemapfel commented Nov 1, 2023

@imciner2 is there something else I can do? I'm waiting on opening a PR with the Julia registry for a package that depends on this jll, but I cannot do so until this is merged and online.

@imciner2 imciner2 merged commit c8b734f into JuliaPackaging:master Nov 1, 2023
@imciner2
Copy link
Member

imciner2 commented Nov 1, 2023

This is good now. It should appear in the registry in a few hours.

@Clemapfel
Copy link
Contributor Author

thank you!

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.

6 participants