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

[lrslib] Add LibraryProduct for liblrsnash #2862

Merged
merged 5 commits into from
Apr 21, 2021

Conversation

bzinberg
Copy link
Contributor

Adds a LibraryProduct for liblrsnash, which will allow this JLL to be used by JuliaPolyhedra/LRSLib.jl.

x-ref: JuliaPolyhedra/LRSLib.jl#39

@bzinberg
Copy link
Contributor Author

@giordano, all the builds succeed on my machine, though it looks like some of them failed in CI. Any idea what's happening?

@giordano
Copy link
Member

giordano commented Apr 20, 2021

all the builds succeed on my machine

What version of Julia and BinaryBuilder are you using? We use Julia v1.6.0 with the environment in the .ci/ directory, I'd recommend you do instantiate that one if you want to debug the issue.

Any idea what's happening?

At a quick glance,
https://github.com/JuliaPolyhedra/lrslib/blob/d8b723a2c315614979a8354f9e768d273d14a215/lrsnashlib.h#L70
https://github.com/JuliaPolyhedra/lrslib/blob/d8b723a2c315614979a8354f9e768d273d14a215/lrsnashlib.c#L51
look like problematic. However I don't have the time to dig this further.

Edit: or maybe the issue is that the global variable in the header file is being defined twice in two different object files. It's just a completely bad idea to define variables in header files (without being extern at least).

@bzinberg
Copy link
Contributor Author

What version of Julia and BinaryBuilder are you using? We use Julia v1.6.0 with the environment in the .ci/ directory, I'd recommend you do instantiate that one if you want to debug the issue.

Will do, thanks for the pointer. I'm currently on Julia 1.5.3 and some less-than-current version of BinaryBuilder.

At a quick glance,

Yeah, it looked to me like clang's complaint was probably valid and just being let slide by the GNU toolchain. I'm going to first try just setting the mac and freebsd toolchains to gnu and see if that makes CI pass, and move on to the linkage bug if that doesn't work.

@giordano
Copy link
Member

You may have missed the edit:

Edit: or maybe the issue is that the global variable in the header file is being defined twice in two different object files. It's just a completely bad idea to define variables in header files (without being extern at least).

I think clang is being more strict, but also more correct. It looks like the source code is simply wrong.

@bzinberg
Copy link
Contributor Author

Edit: or maybe the issue is that the global variable in the header file is being defined twice in two different object files. It's just a completely bad idea to define variables in header files (without being extern at least).

I think clang is being more strict, but also more correct. It looks like the source code is simply wrong.

Yeah, I'd never seen a non-static non-extern global variable declaration in a header file before (though, not an experienced C programmer), had to look up what it means. External linkage and static storage duration. I still want to see if using the GNU toolchain makes CI pass, though I agree that it would only be passing due to undefined behavior.

@bzinberg
Copy link
Contributor Author

bzinberg commented Apr 20, 2021

Switching to GNU toolchain fixed CI. I think pending the above comment about LDFLAGS, this may be ready to merge?

@bzinberg
Copy link
Contributor Author

@giordano, are we ready to merge now or do you think this PR needs to wait on JuliaPolyhedra/lrslib#2? If needs to wait, then @blegat I suggest we merge JuliaPolyhedra/lrslib#3 so that the migration to BinaryBuilder isn't delayed.

@giordano
Copy link
Member

Ok, let's go with this one, but a proper fix upstream rather than ignoring the issue would be better 🙂

@giordano giordano merged commit 645bce8 into JuliaPackaging:master Apr 21, 2021
@bzinberg
Copy link
Contributor Author

Thanks! And a fix is under way in JuliaPolyhedra/lrslib#3!

bzinberg added a commit to bzinberg/Yggdrasil that referenced this pull request Apr 22, 2021
JuliaPolyhedra/LRSLib.jl#40 CI is passing on version [0.1.0+3](JuliaPackaging#2862) of `lrslib_jll`, and fails on earlier versions due to the lack of an `liblrsnash` LibraryProduct.  AFAIK, Julia package compatibility [specifiers](https://pkgdocs.julialang.org/v1/compatibility/) don't have a way of saying "version 0.1.0+3 or above," so we need a version bump to be able to require at least this version in LRSLib.jl's `Project.toml` (see JuliaPolyhedra/LRSLib.jl#40).  Since this release adds more API (in the form of the new LibraryProduct), I think a bump to the [minor version](https://semver.org/#spec-item-7) makes sense, i.e., to `v"0.2.0"`
@bzinberg bzinberg mentioned this pull request Apr 22, 2021
giordano pushed a commit that referenced this pull request Apr 22, 2021
JuliaPolyhedra/LRSLib.jl#40 CI is passing on version [0.1.0+3](#2862) of `lrslib_jll`, and fails on earlier versions due to the lack of an `liblrsnash` LibraryProduct.  AFAIK, Julia package compatibility [specifiers](https://pkgdocs.julialang.org/v1/compatibility/) don't have a way of saying "version 0.1.0+3 or above," so we need a version bump to be able to require at least this version in LRSLib.jl's `Project.toml` (see JuliaPolyhedra/LRSLib.jl#40).  Since this release adds more API (in the form of the new LibraryProduct), I think a bump to the [minor version](https://semver.org/#spec-item-7) makes sense, i.e., to `v"0.2.0"`
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