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

geogram 1.6.6 (new formula) #31094

Closed
wants to merge 2 commits into from
Closed

Conversation

ryanfb
Copy link
Contributor

@ryanfb ryanfb commented Aug 14, 2018

This adds Geogram, a programming library of geometric algorithms. This
is a dependency for programs such as AliceVision / Meshroom
(https://alicevision.github.io/).

  • 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?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

homepage "http://alice.loria.fr/software/geogram/doc/html/index.html"
url "https://gforge.inria.fr/frs/download.php/file/37635/geogram_1.6.6.tar.gz"
sha256 "08211b1d6f21e14701e3fd5c79adbe331cdf66b8af84efdb54cd7048244691b5"
depends_on "cmake" => :build
Copy link
Member

Choose a reason for hiding this comment

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

Should be an empty line between sha256 and the first depends_on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

# don't write directly into /usr/local/lib/cmake/modules
inreplace "CMakeLists.txt", "lib/cmake/modules", "#{share}/cmake/Modules"
# workaround for undefined _mm_set_pd1 in clang
inreplace "src/lib/geogram/numerics/predicates.cpp", "_mm_set_pd1", "_mm_set1_pd"
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 something we're going to need upstream to handle, rather than fixing it ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I only need to try to get upstream to handle the _mm_set_pd1 issue, or to also make the CMake module location configure-time configurable?

Copy link
Member

Choose a reason for hiding this comment

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

Both would be nice 😸, but the _mm_set_pd1 issue is the one I was referencing here.

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 _mm_set_pd1 issue should be fixed upstream in the next release: https://lists.gforge.inria.fr/pipermail/geogram-users/2018-August/000158.html

f << "\nset(CMAKE_INSTALL_PREFIX #{prefix})\n"
end
system "sh", "-f", "configure.sh"
Dir.chdir("build/Darwin-clang-dynamic-Release")
Copy link
Member

Choose a reason for hiding this comment

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

cd do is the preferred syntax in core.

end

test do
system "curl", "https://raw.githubusercontent.com/FreeCAD/Examples/master/Point_cloud_ExampleFiles/PointCloud-Data_Stanford-Bunny.asc", "-o", "bunny.xyz"
Copy link
Member

Choose a reason for hiding this comment

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

Use a resource do block in the main body and then stage this in test do that way. It'll need to be pinned to a commit though, not simply fetching master.

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've made a change for this - I'm not sure if there's a more idiomatic way than File.rename inside a resource stage block to handle the URL filename differing from the needed filename (the test needs the .xyz extension instead of .asc).

@DomT4 DomT4 added the new formula PR adds a new formula to Homebrew/homebrew-core label Aug 14, 2018
inreplace "CMakeLists.txt", "lib/cmake/modules", "#{share}/cmake/Modules"
# workaround for undefined _mm_set_pd1 in clang
inreplace "src/lib/geogram/numerics/predicates.cpp", "_mm_set_pd1", "_mm_set1_pd"
open("CMakeOptions.txt", "a") do |f|
Copy link
Member

Choose a reason for hiding this comment

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

Should be able to use (buildpath/"CMakeOptions.txt").append_lines "blah" or the <<~EOS format of that same helper here.


test do
resource("bunny").stage do
File.rename("PointCloud-Data_Stanford-Bunny.asc", "bunny.xyz")
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably do:

testpath.install resource("bunny")
mv "PointCloud-Data_Stanford-Bunny.asc", "bunny.xyz"
system "#{bin}/vorpalite", "profile=reconstruct", "bunny.xyz", "bunny.meshb"
assert_predicate testpath/"bunny.meshb", :exist?, "bunny.meshb should exist!"

or something.

@ryanfb
Copy link
Contributor Author

ryanfb commented Aug 17, 2018

Thanks for the great feedback, I've re-pushed with those changes!

ryanfb and others added 2 commits August 26, 2018 04:23
This adds Geogram, a programming library of geometric algorithms. This
is a dependency for programs such as AliceVision / Meshroom
(https://alicevision.github.io/).
@DomT4
Copy link
Member

DomT4 commented Aug 26, 2018

Made a few changes, which you can see in this PR preserved as a separate commit, and merged as one in acb53ac. Thank you for your contribution to Homebrew @ryanfb; we appreciate it! 😺

@lock lock bot added the outdated PR was locked due to age label Sep 25, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants