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

qhull 2020.1 #56273

Closed
wants to merge 5 commits into from
Closed

qhull 2020.1 #56273

wants to merge 5 commits into from

Conversation

samford
Copy link
Member

@samford samford commented Jun 14, 2020

  • 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>)?

This bumps the formula to the latest version (2020.1) and also adds the GitHub repo as head (it builds fine without any special modifications). [I removed the previous patch because it's incorporated into this version and no longer applicable.]

It was necessary to add ENV.cxx11 to install, since parts of qhull now require C++ 11 and the build will fail otherwise. I updated the build steps to align more with what's described in the qhull documentation while I was at it.

I've built both version 2020.1 and HEAD from source and tested both and it all looks good. Let me know if any of this could be improved or if anything needs to be changed.

@dawidd6
Copy link
Member

dawidd6 commented Jun 14, 2020

==> brew audit --skip-style visp --online --git
==> FAILED
Error: 1 problem in 1 formula detected
visp:
  * Files were found with references to the Homebrew shims directory.
    The offending files are:
      lib/libvisp_core.3.3.0.dylib

@dawidd6 dawidd6 added the audit failure CI fails while auditing the software label Jun 14, 2020
@samford
Copy link
Member Author

samford commented Jun 14, 2020

Oh dear, it looks like I'm going to have to build visp from source locally to understand what's happening there. I was really hoping to avoid it since there's such an absurd number of dependencies.

Looks like we'll also need revision bumps for octave and pcl. It's interesting that they didn't show up in the previous run.

@samford
Copy link
Member Author

samford commented Jun 14, 2020

The bit with shims references from strings lib/libvisp_core.3.3.0.dylib is as follows:

==========================================================
General configuration information for ViSP 3.3.0
  Version control:               unknown
  Platform:
    Timestamp:                   2020-06-14T14:52:43Z
    Host:                        Darwin 19.5.0 x86_64
    CMake:                       3.17.3
    CMake generator:             Unix Makefiles
    CMake build tool:            /usr/local/Homebrew/Library/Homebrew/shims/mac/super/gmake
    Configuration:               Release
  C/C++:
    Built as dynamic libs?:      yes
    C++ Compiler:                /usr/local/Homebrew/Library/Homebrew/shims/mac/super/clang++  (ver 11.0.3.11030032)
    C++ flags (Release):         -Wall -Wextra -std=c++11 -fvisibility=hidden -msse2 -msse3 -mssse3 -fPIC -DNDEBUG
    C++ flags (Debug):           -Wall -Wextra -std=c++11 -fvisibility=hidden -msse2 -msse3 -mssse3 -fPIC -g
    C Compiler:                  /usr/local/Homebrew/Library/Homebrew/shims/mac/super/clang
    C flags (Release):           -Wall -Wextra -std=c++11 -fvisibility=hidden -msse2 -msse3 -mssse3 -fPIC -DNDEBUG
    C flags (Debug):             -Wall -Wextra -std=c++11 -fvisibility=hidden -msse2 -msse3 -mssse3 -fPIC -g
    Linker flags (Release):
    Linker flags (Debug):

I'm not sure how this is usually resolved in formulae, so any help would be appreciated.

@bayandin
Copy link
Member

bayandin commented Jun 16, 2020

==========================================================
General configuration information for ViSP 3.3.0
  Version control:               unknown
  Platform:
    Timestamp:                   2020-06-14T14:52:43Z
    Host:                        Darwin 19.5.0 x86_64
    CMake:                       3.17.3
    CMake generator:             Unix Makefiles
    CMake build tool:            /usr/local/Homebrew/Library/Homebrew/shims/mac/super/gmake
    Configuration:               Release
  C/C++:
    Built as dynamic libs?:      yes
    C++ Compiler:                /usr/local/Homebrew/Library/Homebrew/shims/mac/super/clang++  (ver 11.0.3.11030032)
    C++ flags (Release):         -Wall -Wextra -std=c++11 -fvisibility=hidden -msse2 -msse3 -mssse3 -fPIC -DNDEBUG
    C++ flags (Debug):           -Wall -Wextra -std=c++11 -fvisibility=hidden -msse2 -msse3 -mssse3 -fPIC -g
    C Compiler:                  /usr/local/Homebrew/Library/Homebrew/shims/mac/super/clang
    C flags (Release):           -Wall -Wextra -std=c++11 -fvisibility=hidden -msse2 -msse3 -mssse3 -fPIC -DNDEBUG
    C flags (Debug):             -Wall -Wextra -std=c++11 -fvisibility=hidden -msse2 -msse3 -mssse3 -fPIC -g
    Linker flags (Release):
    Linker flags (Debug):

It looks it's safe just to inreplace these lines in CMakeLists:

@samford
Copy link
Member Author

samford commented Jun 16, 2020

It looks it's safe just to inreplace these lines in CMakeLists

Is it possible to substitute information that would be appropriate (not just for macOS) instead of simply removing it?

That is to say, we could blank the information like:

    # Avoid superenv shim references
    inreplace "CMakeLists.txt" do |s|
      s.sub!(/CMake build tool:"\s+\${CMAKE_BUILD_TOOL}/, "CMake build tool:\"")
      s.sub!(/C\+\+ Compiler:"\s+\${VISP_COMPILER_STR}/, "C++ Compiler:\"")
      s.sub!(/C Compiler:"\s+\${CMAKE_C_COMPILER}/, "C Compiler:\"")
    end

but I'm not sure whether that would be the best way of dealing with this.

simgrid hardcodes /usr/bin/clang and /usr/bin/clang++ instead of CMAKE_C_COMPILER and CMAKE_CXX_COMPILER but would these always be the same, even on Linux?

@bayandin
Copy link
Member

It looks it's safe just to inreplace these lines in CMakeLists

Is it possible to substitute information that would be appropriate (not just for macOS) instead of simply removing it?

That is to say, we could blank the information like:

    # Avoid superenv shim references
    inreplace "CMakeLists.txt" do |s|
      s.sub!(/CMake build tool:"\s+\${CMAKE_BUILD_TOOL}/, "CMake build tool:\"")
      s.sub!(/C\+\+ Compiler:"\s+\${VISP_COMPILER_STR}/, "C++ Compiler:\"")
      s.sub!(/C Compiler:"\s+\${CMAKE_C_COMPILER}/, "C Compiler:\"")
    end

but I'm not sure whether that would be the best way of dealing with this.

simgrid hardcodes /usr/bin/clang and /usr/bin/clang++ instead of CMAKE_C_COMPILER and CMAKE_CXX_COMPILER but would these always be the same, even on Linux?

Given these lines has an informational purpose only and not used anywhere, I would use the same approach as in simgrid (use relevant binaries from /usr/bin/).

@Moisan Moisan removed the audit failure CI fails while auditing the software label Jun 18, 2020
@BrewTestBot
Copy link
Member

:shipit: @samford has triggered a merge.

@samford
Copy link
Member Author

samford commented Jun 18, 2020

Thanks for the help, @bayandin!

@samford samford deleted the qhull-2020.1 branch June 18, 2020 22:17
@dtrodrigues dtrodrigues mentioned this pull request Aug 4, 2020
5 tasks
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.

5 participants