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
Use cross-compilation flags when checking for system directories #3894
Use cross-compilation flags when checking for system directories #3894
Conversation
mesonbuild/compilers/c.py
Outdated
def get_library_dirs(self): | ||
key = tuple(self.exelist) | ||
def get_library_dirs_real(self, env): | ||
code='' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Flake8]
[E225] missing whitespace around operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I didn't run flake8 and clean up the code much, as I'm looking for initial feedback as to whether this a correct approach or not to fixing the issues. [Hence "DRAFT" in the title :-)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments are automated...
Seems related to bug #3720? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mesonbuild/compilers/c.py
Outdated
paths = [os.path.realpath(p) for p in libstr.split(':') if os.path.exists(os.path.realpath(p))] | ||
return paths | ||
|
||
def get_library_dirs(self, env=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is get_library_dirs()
called from anywhere that doesn't have access to the environment? If no, we should make this a required argument so we don't have hidden bugs. Then you can also remove the conditional below in _get_compiler_check_args()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is generally available, except in the case for mesonbuild/backend/backends.py, where we are filtering out rpath dirs which are in the system directory path. There _libdir_is_system calls get_library_dirs() without access to any environment. [That in turn is called by rpaths_for_bundled_shared_libraries which similarly doesn't have any env info]
As an aside: Is there a reason why the cross build compiler flags are stored in an environment object, rather than in a compiler object for the cross-compiler in question? I would have thought that for cross builds we would have multiple compiler objects, each tracking it's own args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside: Is there a reason why the cross build compiler flags are stored in an environment object, rather than in a compiler object for the cross-compiler in question?
Just cruft from ages past. I think someone recently opened a PR to change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mesonbuild/backend/backends.py
The Backend
object has the environment object available as self.environment
, so you should pass that.
mesonbuild/compilers/compilers.py
Outdated
def get_library_dirs(self, env=None): | ||
if env is None: | ||
env = os.environ.copy() | ||
env['LC_ALL'] = 'C' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a totally different environment
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. However, in the case we don't have an environment available, we need some fallback scheme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is, that the env
passed to get_library_dirs()
in compilers/c.py
is an Environment
object, while this is os.environ
(a dict). We should not conflate the two. Just rename this to osenv
or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, better yet, if you change this function to use _build_wrapper()
like c.py
, this is no longer a problem.
mesonbuild/compilers/compilers.py
Outdated
def get_library_dirs(self, env=None): | ||
if env is None: | ||
env = os.environ.copy() | ||
env['LC_ALL'] = 'C' | ||
stdo = Popen_safe(self.exelist + ['--print-search-dirs'], env=env)[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will want to change this to also use _build_wrapper()
and the get_program_dirs()
below too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
mesonbuild/compilers/c.py
Outdated
return paths | ||
|
||
def get_library_dirs(self, env=None): | ||
key = tuple(self.exelist) # todo include environment here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help requested as to how to manage the environment here. Is it possible to hash it in with the compiler to save the results for later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Environment
object is hashable, afair
3806b68
to
df3e828
Compare
Well, this is fun. The unit tests are failing in a spectacular fashion, but only if they are run one after the other. Not if they are run individually. Probably because we're messing up some global state. |
This means that we will take into account all the flags set in the cross file when fetching the list of library dirs, which means we won't incorrectly look for 64-bit libraries when building for 32-bit. Signed-off-by: Nirbheek Chauhan <nirbheek@centricular.com> Closes mesonbuild#3881
e586202
to
0c974c2
Compare
Shared libraries may not always be linkable without the use of other libraries, so don't make it a linker error.
14f8deb
to
fc54046
Compare
fc54046
to
388d086
Compare
@mtp401 @ChuckDaniels87 @ignatenkobrain Can you please test this branch to see if that fixes your issues? |
Am I correct in understanding that the underlying problem was that compile/link arguments from the cross file were not being used in library lookup tests? If yes then that is strange because it should do that. Is it only for find_library or are the args missing also from other tests? |
The algorithm for The "try linking to Then, in 0.47, I made PkgConfigDependency start passing We merged his fix which does a link check against the library found after manually searching to ensure that it's the right arch, but that broke searching for pkg-config libraries that have missing symbols, so I papered around it by only doing the link check for libraries found in the system paths, not Then people reported #3881 and #3958, where libraries in the system paths had undefined symbols, which meant we couldn't do a link check at all. That leads us to this fix. Now we use HOWEVER, there is still one piece missing in all this. The next step (which can't go into the stable release, and hence isn't in this PR) which will complete the fix for #3720 is to also collect Note that |
Gaah! I really wish the library situation was not so completely terrible as it is. But given that it is and we can't change it, we need to work with it. Feel free to merge once you are convinced that this works. |
I agree, I really wish we had an introspection API or something for compilers/linkers instead of having to guess/emulate/duplicate their behaviour. Something like "just tell me where this library resolves to". |
When building 32-bit on a 64-bit system the "system directories" for libraries are the 64-bit ones unless the appropriate CFLAGS e.g. -m32, as passed to the compiler. Doing so is tricky, as the flags seem to be stored in the environment, rather than the compiler object. This draft patch is a first-attempt to implement this, and works ok for 32-bit and 64-bit builds on linux using GCC.
If the problem of incorrect system paths can be solved, we may be able to remove the build checks in find_library() and hopefully fix #3881.