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

envconfig: add pkg_config_libdir property #6461

Merged

Conversation

dabrain34
Copy link
Contributor

In order to unify the use of sysroot in the cross-file,
the pkg_config_path can now be passed directly in the file.

@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch 2 times, most recently from b7669c6 to b159271 Compare January 14, 2020 11:58
@xclaesse
Copy link
Member

For cross compilation, it's not PKG_CONFIG_PATH you need to set, but PKG_CONFIG_LIBDIR. The difference is the former is in addition to system paths, the latter is instead of system paths.

pkg-config doc recommends using this wrapper for cross compilation, I think Meson should set the same env vars using the cross file.

#!/bin/sh

SYSROOT=/build/root

export PKG_CONFIG_DIR=
export PKG_CONFIG_LIBDIR=${SYSROOT}/usr/lib/pkgconfig:${SYSROOT}/usr/share/pkgconfig
export PKG_CONFIG_SYSROOT_DIR=${SYSROOT}

exec pkg-config "$@"```

@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch from b159271 to a9f7dbb Compare January 14, 2020 14:30
@dabrain34 dabrain34 changed the title envconfig: add pkg_config_path properties envconfig: add pkg_config_libdir properties Jan 14, 2020
@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch from a9f7dbb to b42009c Compare January 14, 2020 16:09
@xclaesse
Copy link
Member

Implementation looks good to me. It's missing doc (grep for pkg_config_path to add at the same place), release notes, and unit test ( similar to run_unittest.py's test_cross_pkg_config_option() probably).

@xclaesse
Copy link
Member

What the test could do:

  • dependency('zlib', method : 'pkg-config', native : false) should return the system version
  • dependency('zlib', method : 'pkg-config', native : true) should return the cross version, write a dummy zlib.pc in a subdir of the test project and set libdir there, you can verify it's your dummy version by checking the version.

Thinking more about it, I think if pkg_config_libdir is not set in the cross file and PKG_CONFIG_LIBDIR has not been set by the user in the environment, we should set it to empty "" when searching for cross dependencies. Otherwise it will lookup for system dependencies and that's never the right thing to do. Or we should at least warn and tell user to set pkg_config_libdir.

@dabrain34
Copy link
Contributor Author

What the test could do:

* dependency('zlib', method : 'pkg-config', native : false) should return the system version

* dependency('zlib', method : 'pkg-config', native : true) should return the cross version, write a dummy zlib.pc in a subdir of the test project and set libdir there, you can verify it's your dummy version by checking the version.

Thx for the recommandation, I'll try to implement something like that. Is there a simple way to run a single unit test from CLI ?

Thinking more about it, I think if pkg_config_libdir is not set in the cross file and PKG_CONFIG_LIBDIR has not been set by the user in the environment, we should set it to empty "" when searching for cross dependencies. Otherwise it will lookup for system dependencies and that's never the right thing to do. Or we should at least warn and tell user to set pkg_config_libdir.

Yes I agree, we should warn some how the user.

@xclaesse
Copy link
Member

Is there a simple way to run a single unit test from CLI ?

./run_unittests.py TheTestClass.test_function

@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch 2 times, most recently from 8c0af70 to e9adbd6 Compare January 15, 2020 18:00
run_unittests.py Outdated Show resolved Hide resolved
run_unittests.py Outdated Show resolved Hide resolved
@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch from e9adbd6 to 39b771d Compare January 15, 2020 18:03
run_unittests.py Outdated Show resolved Hide resolved
docs/markdown/Cross-compilation.md Outdated Show resolved Hide resolved
Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

I think there's a bug in your code if pkg_config_libdir is a string and not a list. I think it would be best if internally we always represented this as a list of strings or None.

mesonbuild/dependencies/base.py Outdated Show resolved Hide resolved
mesonbuild/dependencies/base.py Outdated Show resolved Hide resolved
mesonbuild/envconfig.py Outdated Show resolved Hide resolved
@xclaesse
Copy link
Member

xclaesse commented Jan 15, 2020

@dabrain34 Oops, I just realised that test_native_dep_pkgconfig() does exactly what you want already. You can reuse the same unit test and just add a case where it generate a cross file with pkgconfig='pkg-config' (instead of cross_pkgconfig.py) and set pkg_config_libdir.

@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch 2 times, most recently from 8109e3c to 9ac521e Compare January 15, 2020 21:15
@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch from 9ac521e to e9636cf Compare January 15, 2020 21:18
mesonbuild/envconfig.py Outdated Show resolved Hide resolved
@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch from e9636cf to 9bf75ce Compare January 15, 2020 21:53
Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

Code looks good, thanks for making changes.

You do need some documentation as @xclaesse said. You'll need a release snippet and to update the general docs.

@dabrain34
Copy link
Contributor Author

Code looks good, thanks for making changes.

You do need some documentation as @xclaesse said. You'll need a release snippet and to update the general docs.

Can you give me more details if I have something special to perform here. I think I already updated the documentation.

@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch from 9bf75ce to 866063e Compare January 16, 2020 14:18
run_unittests.py Outdated Show resolved Hide resolved
@xclaesse
Copy link
Member

Code looks good, thanks for making changes.
You do need some documentation as @xclaesse said. You'll need a release snippet and to update the general docs.

Can you give me more details if I have something special to perform here. I think I already updated the documentation.

The doc is fine, but it's missing a release notes snippet. Add a file into docs/markdown/snippets/

@dabrain34 dabrain34 changed the title envconfig: add pkg_config_libdir properties envconfig: add pkg_config_libdir property Jan 17, 2020
@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch from 866063e to 8513855 Compare January 17, 2020 11:42
Copy link
Member

@xclaesse xclaesse left a comment

Choose a reason for hiding this comment

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

almost there, just small nitpickings. Thanks for your work :D

docs/markdown/snippets/pkg_config_libdir.md Outdated Show resolved Hide resolved
run_unittests.py Outdated Show resolved Hide resolved
run_unittests.py Outdated Show resolved Hide resolved
@xclaesse
Copy link
Member

unit tests are failing, it seems unrelated, but that needs to be checked too :/

@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch from 8513855 to 4d74c86 Compare January 17, 2020 14:23
@xclaesse
Copy link
Member

Looks all good to me now, when CI is green.

In order to unify the use of sysroot in the cross-file,
the pkg_config_libdir can now be passed directly in the file.
@dabrain34 dabrain34 force-pushed the dab_cross_file_pkg_config_path_property branch from 4d74c86 to 640e759 Compare January 22, 2020 14:24
@xclaesse
Copy link
Member

LGTM when CI is green.

For the record, we had to drop the change that uses os.pathsep instead of ':' because it causes CI to fail for obscure reasons. Since it requires more work and is unrelated to this PR, better do it separately.

@xclaesse xclaesse merged commit 958df63 into mesonbuild:master Jan 22, 2020
@keven2116
Copy link

Hi,

This feature is not working on the meson 0.53.2, right?

Best Regards,
Wen

@dabrain34
Copy link
Contributor Author

dabrain34 commented Mar 22, 2020

AFAIK its only in master and planned to be release in 0.54

If you need it, its quite easy to use meson as you checkout the master branch of this repo and make alias to meson.py in this repo.

alias meson=/home/user/DEV/meson/meson.py

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.

None yet

5 participants