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

[meson] Fixes for test and install + meson_test & meson_install #6574

Merged
merged 1 commit into from Mar 5, 2020
Merged

[meson] Fixes for test and install + meson_test & meson_install #6574

merged 1 commit into from Mar 5, 2020

Conversation

TheQwertiest
Copy link
Contributor

@TheQwertiest TheQwertiest commented Feb 20, 2020

Changelog: Feature: Fixed inability to run execute test and install separately, that is, without build step. Added meson_test() method, which executes meson test (compared to ninja test in test()). Added meson_install() method, which executes meson install (compared to ninja install in install()).

Docs: conan-io/docs#1568

Fixed inability to run execute test and install separately, that is, without build step. Added meson_test() method, which executes meson test (compared to ninja test in test()). meson test have multiple options for additional test configuration (https://mesonbuild.com/Unit-tests.html#testing-tool). Added meson_install() method, which executes meson install (compared to ninja install in install()). meson install have multiple options for additional installation configuration (https://mesonbuild.com/Installing.html#custom-install-behaviour).

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@claassistantio
Copy link

@claassistantio claassistantio commented Feb 20, 2020

CLA assistant check
All committers have signed the CLA.

- Fixed inability to run execute `test` and `install` separately, that is without `build` step.
- Added `meson_test` method, which executes `meson test` (compared to `ninja test` in test). `meson test` have multiple options for tests configuration (https://mesonbuild.com/Unit-tests.html).
@TheQwertiest
Copy link
Contributor Author

@TheQwertiest TheQwertiest commented Feb 21, 2020

Ideally (implementations of) test() and install() should be replaced with (implementations of) meson_test() and meson_install(), since the later are backend-agnostic, while the former are restricted to ninja backend only.
But that would be a breaking change, thus I left it as-is.

jgsogo
jgsogo approved these changes Feb 24, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Thanks a lot for your work on this PR, specially for taking care not breaking the existing behavior and writing the docs. Thanks!


The behavior will change: now the barrier self._conanfile.should_build is not taken into account for install and test functions, but it matches current behavior in other build-helpers like CMake and it totally makes sense. I hope noone complains 🤞

About functions meson_test and meson_install, your suggestion should be taken into account for Conan v2, let's open an issue with that.

Thanks again 🎉 !!

@jgsogo jgsogo added this to the 1.23 milestone Feb 24, 2020
@jgsogo jgsogo requested a review from czoido Feb 24, 2020
@memsharded memsharded self-assigned this Mar 2, 2020
@memsharded memsharded requested a review from uilianries Mar 2, 2020
@TheQwertiest
Copy link
Contributor Author

@TheQwertiest TheQwertiest commented Mar 3, 2020

I want to add meson version checking to this PR before merging (in case user has an older version, where new methods are not available).
But I need help, since I'm unfamiliar with conan code: there is a get_version method (meson.py#L214) which can be used to get meson version. Would it be correct to cache it's result during Meson's __init__()? I.e. smth like self._version=get_version().

@memsharded
Copy link
Member

@memsharded memsharded commented Mar 3, 2020

Hi @TheQwertiest

I want to add meson version checking to this PR before merging (in case user has an older version, where new methods are not available).
But I need help, since I'm unfamiliar with conan code: there is a get_version method (meson.py#L214) which can be used to get meson version. Would it be correct to cache it's result during Meson's init()? I.e. smth like self._version=get_version().

Yes, I think that we can assume that a Meson instance doesn't change the meson version during its lifetime, go ahead, please.

def meson_install(self, args=None, build_dir=None):
if not self._conanfile.should_install:
return
self._run_meson_command(subcommand='install', args=args, build_dir=build_dir)
Copy link
Member

@memsharded memsharded Mar 3, 2020

Choose a reason for hiding this comment

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

These commands might not be supported by all meson versions, so @TheQwertiest is suggesting to add a version check to decide behavior. I am fine with that.

@memsharded
Copy link
Member

@memsharded memsharded commented Mar 4, 2020

Hi @TheQwertiest

Tomorrow I will merge this. I think it can be merged even if the version check is not there, after all, they are new methods, very low risk. Version check could be added later, or maybe even not necessary. Do you think that it is very important?

@TheQwertiest
Copy link
Contributor Author

@TheQwertiest TheQwertiest commented Mar 5, 2020

it's not very important, just a better diagnostics message.
I was planning to add those tomorrow, but can push them to a separate PR, if needed.

@memsharded
Copy link
Member

@memsharded memsharded commented Mar 5, 2020

it's not very important, just a better diagnostics message.
I was planning to add those tomorrow, but can push them to a separate PR, if needed.

Cool, I am going to merge this now. If you manage to do it, please open a new PR, thanks!

@memsharded memsharded merged commit a99199c into conan-io:develop Mar 5, 2020
2 checks passed
czoido added a commit to conan-io/docs that referenced this issue Mar 10, 2020
@jgsogo jgsogo mentioned this pull request Mar 11, 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.

None yet

5 participants