Relax restrictions on detecting boost dependency #7909

kjmeagher · 2020-10-28T16:26:24Z

This does two things:

allows the .so files to be symlinks
searches lib and lib64 in BOOST_ROOT even if it finds lib
directories from the compiler

The first condition is needed for the homebrew on macs because certian
boost modules are provided by seperate packages and are put together in
a directory with symlinks to the .so files. The both conditions are
needed for high performace computing environments where dependencies are
often provided in nonstandard directories with symlinks

mensinda

I have nothing against these changes in principle, but the implementation needs some work.

Also, a test case would be nice.

mensinda · 2020-10-28T16:33:45Z

mesonbuild/dependencies/boost.py

-        if system_dirs:
-            return system_dirs
-


Removing this effectively makes the above code where system_dirs useless, which might break existing setups.

I changed it to use both the system_dirs and the hard-coded lib directories

mensinda · 2020-10-28T16:35:42Z

mesonbuild/dependencies/boost.py

@@ -652,7 +649,7 @@ def filter_libraries(self, libs: T.List[BoostLibraryFile], lib_vers: str) -> T.L
    def detect_libraries(self, libdir: Path) -> T.List[BoostLibraryFile]:
        libs = []  # type: T.List[BoostLibraryFile]
        for i in libdir.iterdir():
-            if not i.is_file() or i.is_symlink():


Symlinks were excluded on purpose to avoid duplicate libraries in the detection logic and because symlinks usually remove information. Please either ensure that all symlinks are resolved or that "real" paths of a library are always preferred.

I changed it to resolve all the symlinks and use set() to remove duplicates

Why not make libs into a set from the very beginning?

No good reason. I changed it to use a set from the begining

lgtm-com · 2020-10-28T16:37:38Z

This pull request introduces 1 alert when merging 4305227 into 42d6b37 - view on LGTM.com

new alerts:

1 for Unused local variable

deepbluev7 · 2020-11-05T23:53:32Z

The symlink thing was something that confused me for quite a while. I used cget to build boost, which by default symlinks all libraries into a directory. This means there are only symlinked libraries in the lib folder. I worked around this for now, but this PR would have saved me quite a bit of time. I do agree though, that non-symlinked libraries should be preferred. Thank you for working on this!

lgtm-com · 2021-07-14T15:41:49Z

This pull request introduces 1 alert when merging afe2acc into 762c073 - view on LGTM.com

new alerts:

1 for Unused local variable

codecov · 2021-07-14T15:53:58Z

Codecov Report

Merging #7909 (19d10fe) into master (7dee480) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7909      +/-   ##
==========================================
- Coverage   66.43%   66.43%   -0.01%     
==========================================
  Files         374      374              
  Lines       84329    84325       -4     
  Branches    17557    17555       -2     
==========================================
- Hits        56026    56019       -7     
+ Misses      23383    23379       -4     
- Partials     4920     4927       +7

Impacted Files	Coverage Δ
mesonbuild/dependencies/boost.py	`83.61% <100.00%> (+0.56%)`	⬆️
mesonbuild/scripts/vcstagger.py	`87.50% <0.00%> (-4.17%)`	⬇️
mesonbuild/dependencies/ui.py	`69.75% <0.00%> (-2.47%)`	⬇️
mesonbuild/dependencies/dev.py	`69.54% <0.00%> (-1.15%)`	⬇️
mesonbuild/dependencies/base.py	`84.21% <0.00%> (-0.59%)`	⬇️
dependencies/boost.py	`82.26% <0.00%> (+0.98%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dee480...19d10fe. Read the comment docs.

This does two things: * allows the library files to be symlinks * searches `lib` and `lib64` in `BOOST_ROOT` even if it finds lib directories from the compiler The first condition is needed for the homebrew on macOS because boost and boost python are provided in seperate packages and are put together in /usr/local/lib with symlinks to the library files. The both conditions are needed for high performace computing environments where dependencies are often provided in nonstandard directories with symlinks A test case was added which looks for boost libraries in seperate directories which have been symlinked to BOOST_ROOT/lib

mensinda

I just got one more issue, but then this is good to go.

mensinda · 2021-07-15T17:29:03Z

mesonbuild/dependencies/boost.py

@@ -561,7 +557,7 @@ def detect_lib_dirs(self, root: Path) -> T.List[Path]:
        # Filter out paths that don't match the target arch to avoid finding
        # the wrong libraries. See https://github.com/mesonbuild/meson/issues/7110
        if not self.arch:
-            return dirs + subdirs
+            return system_dirs + dirs + subdirs


system_dirs should also be returned here in an elif branch since they would be ignored otherwise and should not be subject to the filtering logic below

After reading #7110 I think I might be going about this wrong let me try a different approach

I re-wrote it to work explicitly what I was trying to do. Now when searching BOOST_ROOT it won't use the system_dirs, but when looking in other directories like /usr/local it will

eli-schwartz · 2021-07-15T20:04:13Z

mesonbuild/dependencies/boost.py

                continue
            if not any([i.name.startswith(x) for x in ['libboost_', 'boost_']]):
                continue

-            libs += [BoostLibraryFile(i)]
-        return [x for x in libs if x.is_boost()]  # Filter out no boost libraries
+            libs.add(i.resolve())


When I suggested making it into a set from the beginning, I was kind of assuming the logical next step is to consolidate this. You no longer need multiple complex filtering passes, so...

Suggested change

libs.add(i.resolve())

libs.add(BoostLibraryFile(i.resolve()))

And then you don't need to assign blibs and make the return line refer to a different variable. This would help reduce the size of the diff... :)

I made the requested change

This boolean parameter is added to check_and_set_roots() and detect_lib_dirs() when true it will first search the compiler library directories before checking the standard lib directories. This is set to false when using BOOST_ROOT and set to true when useing other system directories like /usr/local Also simplify using a set to remove duplicate lib files Also remove the part where you search the Cellar in homebrew, this is unnescessary now that symlinks can be followed it should find boost when searching /usr/local so no need to search the Cellar

mensinda · 2021-07-16T20:03:26Z

mesonbuild/dependencies/boost.py

-            # Homebrew
-            brew_boost = Path('/usr/local/Cellar/boost')
-            if brew_boost.is_dir():
-                tmp += [x for x in brew_boost.iterdir()]
-


What is the reason behind removing this?

On homebrew systems, boost should be detected in /usr/local/lib instead of the cellar. Looking in the cellar made sense before when the logic couldn't follow symlinks. But since this patch adds support for following symlinks, it now finds boost in /usr/local/lib like it should. In addition, event if it somehow got to checking the cellar it wouldn't find boost_python because it is in a different homebrew formula.

This currently uses my fork of meson but it should be changed to the official repo when my PR is accepted. See mesonbuild/meson#7909

Previously the meson test case would only test boost-python on linux. With the #7909 it is now possible to use boost-python on macOS/homebrew. This enables the boost-python test on both linux and macOS. It also uses python.extension_module() instead of shared_library to make the python extension module.

kjmeagher requested a review from jpakkane as a code owner October 28, 2020 16:26

mensinda added boost dependencies labels Oct 28, 2020

kjmeagher mentioned this pull request Oct 28, 2020

Meson build system can't find boost icecube/nuflux#6

Closed

mensinda requested changes Oct 28, 2020

View reviewed changes

kjmeagher marked this pull request as draft July 14, 2021 14:38

kjmeagher added 2 commits July 15, 2021 10:06

Make libs a set from the begining

ea2b725

kjmeagher marked this pull request as ready for review July 15, 2021 15:58

mensinda reviewed Jul 15, 2021

View reviewed changes

eli-schwartz reviewed Jul 15, 2021

View reviewed changes

mensinda reviewed Jul 16, 2021

View reviewed changes

mensinda approved these changes Jul 17, 2021

View reviewed changes

kjmeagher added a commit to icecube/nuflux that referenced this pull request Jul 19, 2021

Add workflow for macOS build with meson

8cb6f42

This currently uses my fork of meson but it should be changed to the official repo when my PR is accepted. See mesonbuild/meson#7909

mensinda merged commit 3c3fa0a into mesonbuild:master Aug 4, 2021

kjmeagher mentioned this pull request Aug 11, 2021

Test boost-python on macOS #9099

Merged

eli-schwartz mentioned this pull request Oct 11, 2024

1.6.0rc1 regression: boost test fails (worked with 1.5.2) #13763

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax restrictions on detecting boost dependency #7909

Relax restrictions on detecting boost dependency #7909

kjmeagher commented Oct 28, 2020

mensinda left a comment

mensinda Oct 28, 2020

kjmeagher Jul 15, 2021

mensinda Oct 28, 2020

kjmeagher Jul 15, 2021

eli-schwartz Jul 15, 2021

kjmeagher Jul 15, 2021

lgtm-com bot commented Oct 28, 2020

deepbluev7 commented Nov 5, 2020 •

edited

Loading

lgtm-com bot commented Jul 14, 2021

codecov bot commented Jul 14, 2021 •

edited

Loading

mensinda left a comment

mensinda Jul 15, 2021

kjmeagher Jul 16, 2021

kjmeagher Jul 16, 2021

eli-schwartz Jul 15, 2021

kjmeagher Jul 16, 2021

mensinda Jul 16, 2021

kjmeagher Jul 16, 2021

	libs.add(i.resolve())
	libs.add(BoostLibraryFile(i.resolve()))

Relax restrictions on detecting boost dependency #7909

Relax restrictions on detecting boost dependency #7909

Conversation

kjmeagher commented Oct 28, 2020

mensinda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Oct 28, 2020

deepbluev7 commented Nov 5, 2020 • edited Loading

lgtm-com bot commented Jul 14, 2021

codecov bot commented Jul 14, 2021 • edited Loading

Codecov Report

mensinda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepbluev7 commented Nov 5, 2020 •

edited

Loading

codecov bot commented Jul 14, 2021 •

edited

Loading