Skip to content
This repository has been archived by the owner on Dec 14, 2020. It is now read-only.

Fix build from source with Clang v4 #37

Merged
merged 1 commit into from
Mar 2, 2018
Merged

Fix build from source with Clang v4 #37

merged 1 commit into from
Mar 2, 2018

Conversation

jgoldfar
Copy link
Contributor

(makes ordered comparison between pointer and zero an error)

@jgoldfar
Copy link
Contributor Author

Not sure how this would be tested through CI; would perhaps be worthwhile to be able to select a BinDeps provider manually and test more combinations? Homebrew install was not working locally, hence this fix

@tkelman
Copy link
Contributor

tkelman commented Mar 2, 2018

This isn't the right fix, the comparison shouldn't be on the pointer but on the dereferenced value, as was done upstream in coin-or/SYMPHONY@907c0be

@jgoldfar
Copy link
Contributor Author

jgoldfar commented Mar 2, 2018

Fair enough (and evident in retrospect.) Worth updating this PR or close?

@tkelman
Copy link
Contributor

tkelman commented Mar 2, 2018

Happy to merge if you want to update, whether it's worth it is up to you. See discussion in #44

@jgoldfar
Copy link
Contributor Author

jgoldfar commented Mar 2, 2018

PR is updated (but this code path remains untested in any automated way...)

@tkelman
Copy link
Contributor

tkelman commented Mar 2, 2018

Suppose the homebrew provider could be put behind an env var flag so it would be possible to disable without needing code modifications, if you wanted to get fancy

@jgoldfar
Copy link
Contributor Author

jgoldfar commented Mar 2, 2018

My comment above is mostly a dupe of JuliaPackaging/BinDeps.jl#334, but I wonder what the point of such an option would be if not also tested. Not that every project even could build on e.g. Travis before timing out; there's definitely merit to having one provider that works for 99% of cases. Looking forward to BinaryBuilder making our lives easier!
PS many thanks for your contributions to Julia and the surrounding ecosystem

@tkelman tkelman merged commit 6b85bd6 into JuliaOpt:master Mar 2, 2018
@tkelman
Copy link
Contributor

tkelman commented Mar 2, 2018

You would want to test things naturally, but Travis can't really do some of the more obscure cases like freebsd, and non-ubuntu distros or non-default configurations of julia would require more docker work than most people probably want to undertake.

Thanks for the kind words. I'm hardly involved much any more, hence the few month gap where I wasn't looking at things like this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants