I would NOT accept it.
Why? Answer: source role is not for building package in CI-PIPELENES.
We had discussion with @bookwar, and other and we decided to no encourage get .SRMS until this is really necessary. For example .SRPM has tests. Otherwise developers misunderstand purpose of this role.
--> Why to install BuilRequires to test-environment otherwise as for build package?

Other reason NOT to merge: some buildrequires are ONLY available in BuildRoot and ABSENT in regular repos.

We need to able to fetch the sources and run tests that are located there, like make check.

What do you recommend us to do?

Note, this role doesn't build a package, but needs to run rpmbuild -bp ...

@bookwar @mvadkert could you provide your feedback here, pls?

The role want's to run build prepare command but can not do that becuase a dependency is required even in the preparation phase ...

Is python3-devel a common package required in the prep phase? If so maybe we can just install it as a STR dependency, insteda of installing all deps on test runner. This approach might be an overkill ...

Maybe even introducing required_packages to standard-test-source is a more sane approach ...

okay, I guess that should be fine, the developer would just need to know what he needs to add to run rpmbuild -bp.

@astepano would you be okay with it?

yes, required_packages is great idea.

Please, no rpmbuild -bp in ansible playbooks/roles

I've updated to use required_packages, but I don't understand what is the problem with rpmbuild -bp. What do you suggest to use instead?

FWIW, i would really prefer that we be able to use dnf --install-deps as a way to install the needed build dependencies, as using required_packages implies we need to keep that list in sync with the BuildRequires tags in the spec file. If we use dnf --install-deps, the dependencies need only be recorded in a single canonical location

+1 to use dnf build-deps as it is the correct solution I believe, but let's discuss on meeting why this is not suggested by Andrei ...

Let's define our target: https://fedoraproject.org/wiki/CI/Standard_Test_Interface#Summary

Quote: On the other hand, integration testing should happen against a composed system.

In my understanding: we test composes.
Purposes of dist-git-tests in RHEL8 is to test composes: BaseOS/AppStream.
We promise and advertise for a long time.
On different events: meetings, conf talks, documents.

Yes. We have buildroot now. But. For a long time we say: this is temporary solution. And must be avoid by all means. It is much easier to prevent to do some modification, that fix it in the future.

You can say: we want to have BuildRequires on test-runner.
Then I will say: BuilRequires can be different on RHEL8 and Fedora28 and Fedora30.
Then I will ask: why do you want to have "BuilRequires" different than test-env ?

@astepano what about if we split this role in 2.

• 1 new role, like fetch-source role. It will just fetch the compressed source
• the source role would then import fetch-source role and will try to extract and run %prep step like it does now.

If some package does require some extra packages to run rpmbuild, then they can just use the fetch-sources directly and do what they need to do with the compressed source.

or fetch_only: True flag to the current role, which would be false by default

also works for me.

@bgoncalv sounds like a good solution.
I would add : will try to extract and run %prep step like it does now + print info to developer about possible limitations of this step

@astepano nice! What do you prefer splitting the role or just using fetch_only flag?

pretty please pagure-ci rebuild

I am not sure about example. This example could be OK. But.... What would be if test-env is container ?

I am not sure about example. This example could be OK. But.... What would be if test-env is container ?

I dind't understand what is your concern?

• dest: "/var/str/"
  is defined var in ./str-common-pkgs/defaults/main.yml:5:tenv_workdir: /var/str/
  User doesn't know about this directory.
  Official example should not recommend use "/var/str/"

dest: "/var/str/"
is defined var in ./str-common-pkgs/defaults/main.yml:5:tenv_workdir: /var/str/
User doesn't know about this directory.
Official example should not recommend use "/var/str/"

I disagree :)

standard-test-basic role is used in many places, so I think it is good to have an example how to use it with source role. To use basic role we need to copy the files to "/var/str/".

If you want we can move the copy file to the source role and we default it to "/var/str/".

So it would work without problems with basic role, but it can make things more confusing when users want define their own role to run the tests.

standard-test-basic role uses macro env_workdir
Who does guaranty that /var/str/ exists ?
/var/str is hidden current implementation of STR.
About it users should not know.

I'm aware of it, that's why in the example I've added a comment at that place, # standard-test-basic directory for tests are relative to /var/str.

If you have a better idea how to do it, please let me know.

just use env_workdir it is official to users to use it.

env_workdir is not defined outside standard-test-basic, but thanks for the tip. I've updated the example to set env_workdir globally to the whole playbook. It works well.

If you like the solution I'll squash the commits.

@bgoncalv hi
Looks very good. Thank you for persistency and updating this PR.
Let's merge it.

rebased and squashed commits.

thanks for insisting in improving the example :)

Commit 4adf316 fixes this pull-request