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

graph-tool.rb: Update to version 2.28 #40825

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@count0
Copy link
Contributor

commented Jun 9, 2019

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Show resolved Hide resolved Formula/graph-tool.rb Outdated
@fxcoudert

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Thanks @count0, can you please rebase this into a single commit of top of current master?

@count0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@fxcoudert Can't you just squash the commits from github's interface when merging?

I'm relying solely on the web interface to do these updates, I admit. But if this is inconvenient for you, I'll go ahead and squash them by hand.

@fxcoudert

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@count0 no, our CI machines cannot test if it does not apply cleanly on top of master. Since we rely on them for testing and also for generation of pre-built binaries (bottles), we cannot do without.

@count0 count0 force-pushed the count0:patch-2 branch from 2162b10 to 6fc3a75 Jun 10, 2019

@count0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@fxcoudert Ok, no problem.

@count0 count0 force-pushed the count0:patch-2 branch from 6fc3a75 to fdd59fd Jun 10, 2019

@count0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

The sierra build seems to be failing due to an old compiler. For high sierra it fails installing matplotlib, not graph-tool proper. Everything seems fine for mojave.

@fxcoudert

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

  • Regarding Sierra, is it documented what clang version is now required for graph-tool?
  • For High Sierra, since matplotlib is part of the graph-tool formula, that is a problem. Maybe a newer version would work better? (all resources should be updated to the latest compatible version)
@count0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Clang 8 is able to compile it, but I'm not sure if any older version can.

I'll try installing a newer matplotlib.

@count0 count0 force-pushed the count0:patch-2 branch 2 times, most recently from 8e0f114 to b7aca44 Jun 10, 2019

@count0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@fxcoudert The CI checks seem to be stuck on pending...

@SMillerDev

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Nah, it just has a very long queue ahead of it because of some CI issues a couple of hours ago. It'll go through eventually

@count0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

So, now we are getting the same errors in sierra and high-sierra due to an outdated compiler.

Is there any way to require clang 8 in these two platforms?

@scpeters

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

high sierra had a different failure due to a problem with the build machine

@BrewTestBot test this please

@scpeters

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

@BrewTestBot test this please

@scpeters

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

can you rebase this on master again? there's something weird happening with the CI

@count0 count0 force-pushed the count0:patch-2 branch from b7aca44 to 550e06e Jun 11, 2019

@count0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

The tests finished suspiciously fast... The build should take about an hour, but it finished in a little over three minutes. Can't seem to get to the build log...

@scpeters

This comment has been minimized.

@scpeters

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

how are you rebasing? it looks like the parent of 550e06e is e8432d9 from 20 hours ago. It would be best if you could rebase off of the latest part of the master branch. I usually do something like the following:

git checkout master
git pull origin master
git checkout patch-2
git rebase -i master

@count0 count0 force-pushed the count0:patch-2 branch from 550e06e to 7290565 Jun 12, 2019

@count0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@scpeters well, that's the amount of time it's passing between the checks. ;-) I'm rebasing once more.

@count0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

The checks were aborted for some reason, and jenkins was shut down.
Let me see if I can do this:

@BrewTestBot test this please

@scpeters

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

it looks like CI ran properly, but the sierra and high-sierra builds failed. high sierra has been failing for a while, but I think the sierra failure is a regression. it's a significant regression too because users on high sierra have been able to use the sierra bottles as a workaround

@count0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@scpeters The failures are on valid C++17 code. This new version of graph-tool requires C++17, and we need a compiler that supports it. This should be (vanilla) clang version 5 or above. I think this means Xcode 9 or above.

@scpeters

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

ok, can you add a line depends_on :macos => :mojave # c++17 to the formula and reflect that in a commit message?

@count0 count0 force-pushed the count0:patch-2 branch from 79e7b6f to 027ffbd Jun 12, 2019

depends_on "numpy"
depends_on "py3cairo"
depends_on "pygobject3"
depends_on "python"
depends_on "scipy"

This comment has been minimized.

Copy link
@scpeters

scpeters Jun 12, 2019

Member

remove this whitespace please

url "https://git.skewed.de/count0/graph-tool/commit/aa39e4a6.diff"
sha256 "5a4ea386342c2de9422da5b07dd4272d47d2cdbba99d9b258bff65a69da562c1"
end

def install
# Work around "error: no member named 'signbit' in the global namespace"
ENV["SDKROOT"] = MacOS.sdk_path if MacOS.version == :high_sierra

This comment has been minimized.

Copy link
@scpeters

scpeters Jun 12, 2019

Member

can you remove these lines as well since :high_sierra is no longer supported?

@fxcoudert

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Thanks @count0, I have made the changes while merging.

@fxcoudert fxcoudert closed this in eed5afc Jun 13, 2019

@scpeters

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

thanks FX!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.