Unset the executable bit on Ansible modules.

[pcahyna]

Per the policy defined in https://docs.ansible.com/ansible/latest/dev_guide/testing/sanity/shebang.html.
(Some RPM checks do not like an executable script to have a #!/usr/bin/python
shebang.)

[thom311]

library/test_network_connections.py is not an ansible python module. It's a unit-test that imports network_connections.py. It should be executable.
Also, it already has #!/usr/bin/env python, so I don't think

(Some RPM checks do not like an executable script to have a #!/usr/bin/python shebang.)

applies. Is the unit-test even packaged in the RPM? It probably should not, should it?

[pcahyna]

Thanks for the explanations. I have a few comments:

library/test_network_connections.py is not an ansible python module. It's a unit-test that imports network_connections.py.

If it is not a module, it should not be, IMO, in library/ b/c this subdirectory is meant to be used for embedded modules and using it for other scripts is confusing. I checked how it is used; it seems to be referenced only in a copy task in test/test_unit.yml. In that case I think it is better to put it under files. This is the default directory for files referenced in copy tasks: http://docs.ansible.com/ansible/latest/user_guide/playbooks_reuse_roles.html#using-roles :

Any copy, script, template or include tasks (in the role) can reference files in roles/x/{files,templates,tasks}/ (dir depends on task) without having to path them relatively or absolutely.

This will actually simplify the copy task: no need to reference this special /role mount, so it will be easier to perhaps run the tests w/o docker in the future.

Anyway that was rather a digression. Back to the topic:

It should be executable.

In test/test_unit.yml, the script is always executed using the python interpreter explicitely, so it does not need to be executable. This is quite fundamental, because the basic function of the test is to execute the script both with python2 and python3 (which is, by the way, a very good approach, thank you for that). So executing the script by making it executable & a shebang line will not work properly. I even think that the script does not need a shebang line at all for this reason. The fact that the script does not need to be executable can be seen also from the success of the tests on this pull request.

Also, it already has #!/usr/bin/env python, so I don't think

(Some RPM checks do not like an executable script to have a #!/usr/bin/python shebang.)

applies.

Unfortunately, the check actually checks whether the shebang invokes python or pythonN. Whether it invokes it directly or via /usr/bin/env does not matter. The checking script is smart enough to transform /usr/bin/env to a direct call. So, #!/usr/bin/env python is wrong, while #!/usr/bin/env python3 would be OK. The source code for this check can be found here: https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/master/f/brp-mangle-shebangs

Is the unit-test even packaged in the RPM? It probably should not, should it?

It is; the test/ subdirectory is as well and the unit test is referenced there (and the test is actually executed in the CI), so it should stay this way.

[thom311]

why does the script reject pure "python"? What is a script supposed to specify, that works both with python2 and python3?

PEP394 remarks "... python should be used in the shebang line only for scripts that are source compatible with both Python 2 and 3.".

[pcahyna]

From what I understood, the point of the check is to make sure that nothing in Fedora uses python - it should be specified explicitly which version of Python is going to be used, even if the script is compatible with both, because even in this case it's better to use and test it with one supported interpreter only. Also, the Python maintainers say that the "verified to work with both" case is almost undistinguishable from "maintainer didn't think about the problem", making it quite hard for them to manage. Anyway, this script is a bit a special case, because it is actually not only compatible with both Pythons but intended to be executed using both in the same test. A shebang line can not accomplish that, so the test invokes the interpreter. But then, the script is not meant to be executed directly, only using /usr/bin/python[23] test_network_connections.py, so it does not need to be executable (and does not even need a shebang line).
I therefore propose to drop the executable bit, which will make this intention clear and also solve the problem with the check. The shebang line could be dropped as well.
By the way, why does test_unit.yml run the script using python2, then python3 and finally python? The first two should be enough. (I am asking because using just python is exactly what Fedora wants to avoid (not only in shebangs but everywhere) and there is no guarantee that /usr/bin/python will continue to exist actually.)

[pcahyna]

no need to reference this special /role mount, so it will be easier to perhaps run the tests w/o docker in the future.

Sorry, I was looking at an older version of the tests, please disregard that part. The point about a file which is only referenced in a copy task belonging to files/ and not to library/ still applies though.

[pcahyna]

I split the commit in two because the two files are a bit different cases.

[thom311]

the commit message says:

This script is not being executed directly,

this is not correct. I run the unit-tests manually all the time (without explicitly selecting the interpreter, which is the entire point of the shebang).

The Fedora script warns when a shebang contains python alone. I think that is wrong, or at least overly strict.
Anyway, the patch doesn't fix any problem, it just shuts up the warning. Why can you not instead just ignore the warning? It's just a (wrong) warning. Either ignore it, or the RPM script should support a way to disable it case the user knows what he is doing.

Note that the RPM script is only for Fedora 28 or newer. This is the upstream project, if you need to patch anything to make Fedora 28+ build scripts happy, then the right place is downstream.

[pcahyna]

the commit message says:

This script is not being executed directly,

this is not correct. I run the unit-tests manually all the time (without explicitly selecting the interpreter, which is the entire point of the shebang).

OK, beware though that /usr/bin/python may cease to exist in future Fedoras.

It's just a (wrong) warning.

I am not the right person to complain about that, but it seems that this error (not warning) has justification, if the intent is to remove /usr/bin/python in the future.

Note that the RPM script is only for Fedora 28 or newer. This is the upstream project, if you need to patch anything to make Fedora 28+ build scripts happy, then the right place is downstream.

I will do that, in general since the project is mostly focused on Fedora-derived distributions (Fedora, RHEL, CentOS), it makes sense to minimize differences between upstream and downstream.

Can you at least merge the first commit, which has been uncontroversial so far?

[pcahyna]

I am not the right person to complain about that, but it seems that this error (not warning) has justification, if the intent is to remove /usr/bin/python in the future.

To clarify that, in Fedora it is currently still a warning, but it says This will become an ERROR, fix it manually! and it has already become in a version used internally.

[thom311]

OK, beware though that /usr/bin/python may cease to exist in future Fedoras.

Interesting. Do you have a source for that? It would break countless scripts (and upstream projects). The warning in the RPM script, makes it sound like it will later ERROR instead of WARN when building packages for Fedora. But even then, we couldn't patch the shebang upstream (here), because then it would stop working on RHEL7 and RHEL6.

Can you at least merge the first commit, which has been uncontroversial so far?

yes, first one lgtm. I am pretty sure you have permissions to merge. Please go ahead.

[pcahyna]

OK, beware though that /usr/bin/python may cease to exist in future Fedoras.

Interesting. Do you have a source for that? It would break countless scripts (and upstream projects).

That's how I read the discussion in python/peps#630.

The warning in the RPM script, makes it sound like it will later ERROR instead of WARN when building packages for Fedora. But even then, we couldn't patch the shebang upstream (here), because then it would stop working on RHEL7 and RHEL6.

We can turn it into python2, which exists even on RHEL6. Of course, this has a drawback as well, because then you will be forced to install python2 on newer systems which will not have it by default (and will stop shipping it completely at some point).

[tyll]

IMHO we should move the commands required for packaging into a Makefile or some other install script and then adjust things there when installing to an actual system. I would assume that we do not need any executable flags at all on the installed system and also not shebangs. It is strange for me that https://docs.ansible.com/ansible/latest/dev_guide/testing/sanity/shebang.html requires shebangs for non-executable Ansible modules.

@pcahyna is there a Fedora package for this project? It would be good to call the unit scripts as part of %check there.

[tyll]

If it is not a module, it should not be, IMO, in library/ b/c this subdirectory is meant to be used for embedded modules and using it for other scripts is confusing. I checked how it is used; it seems to be referenced only in a copy task in test/test_unit.yml. In that case I think it is better to put it under files. This is the default directory for files referenced in copy tasks: http://docs.ansible.com/ansible/latest/user_guide/playbooks_reuse_roles.html#using-roles :

The files in test/ are not roles, therefore the info about files does not apply here. It might make sense to move library/test_network_connections.py to the test/ directory to keep all tests data together but this makes running the unit tests during development slightly more complicated.

[tyll]

That's how I read the discussion in python/peps#630.
It also distinguishes between upstream and the distribution which is also an argument to move the logic for the shebang to a Makefile or the spec file:
https://github.com/python/peps/pull/630/files#diff-1d22f7bd72cbc900670f058b1107d426R67

[pcahyna]

IMHO we should move the commands required for packaging into a Makefile or some other install script and then adjust things there when installing to an actual system. I would assume that we do not need any executable flags at all on the installed system and also not shebangs.

We don't indeed.

It is strange for me that https://docs.ansible.com/ansible/latest/dev_guide/testing/sanity/shebang.html requires shebangs for non-executable Ansible modules.

I guess they are treating this shebang line specially (like searching for this exact string and replacing it with the actual interpreter after the module is pushed to the managed host).

@pcahyna is there a Fedora package for this project? It would be good to call the unit scripts as part of %check there.

So far only a RHEL package. The unit test script can be called as part of %check, if it does not make any changes to the system (I haven't looked what it does; the fact that it calls the ansible module looks a bit suspicious). But the unit test script is being executed in the integration tests in every GitHub PR.

[pcahyna]

The files in test/ are not roles, therefore the info about files does not apply here. It might make sense to move library/test_network_connections.py to the test/ directory to keep all tests data together but this makes running the unit tests during development slightly more complicated.

Good point. Still, how does moving it make running the unit tests more complicated?

It also distinguishes between upstream and the distribution which is also an argument to move the logic for the shebang to a Makefile or the spec file:
https://github.com/python/peps/pull/630/files#diff-1d22f7bd72cbc900670f058b1107d426R67

It is certainly possible, my motivation was to keep the differences with upstream at a minimum.

[tyll]

Do we still need to do something here when #46 is merged?

[pcahyna]

No, the first commit was merged separately, the other file will not be touched for now.