openSUSE / python-rpm-macros

Multi-Python, Single-Spec macros generator
Other
22 stars 20 forks source link

Add "export" PYTHONPATH for the correct usage inside of the installation directory ( #124) #125

Closed skriesch closed 2 years ago

skriesch commented 2 years ago

PYTHONPATH has been set at the moment but has not been exported. Therefore, some python tests can not find the installation directory with built modules by the package ( #124 ). In my case, I can not build python-zstandard because of this issue. The "export" has got the effect of the usage of the correct PYTHONPATH in pytest.

bnavigator commented 2 years ago

Hi,

if you want to export, you also have to separate the export from the subsequent pytest call, otherwise you don't call pytest at all.

ben@skylab:~/tmp> cat A.sh 
#!/bin/sh
echo "MYVAR: $MYVAR"
ben@skylab:~/tmp> ./A.sh
MYVAR: 
ben@skylab:~/tmp> MYVAR=myvalue ./A.sh
MYVAR: myvalue
ben@skylab:~/tmp> ./A.sh
MYVAR: 
ben@skylab:~/tmp> export MYVAR=myvalue; ./A.sh
MYVAR: myvalue
ben@skylab:~/tmp> MYVAR=myvalue ./A.sh
MYVAR: myvalue

However, if you export, you accumulate the list, and that's not what you want when you expand for multiple flavors.

ben@skylab:~/tmp> MYVAR=${MYVAR:+$MYVAR:}standardflavorpath ./A.sh
MYVAR: standardflavorpath
ben@skylab:~/tmp> MYVAR=abc
ben@skylab:~/tmp> MYVAR=${MYVAR:+$MYVAR:}standardflavorpath ./A.sh
MYVAR: abc:standardflavorpath
ben@skylab:~/tmp> MYVAR=${MYVAR:+$MYVAR:}standardflavorpath ./A.sh
MYVAR: abc:standardflavorpath
ben@skylab:~/tmp> export MYVAR=${MYVAR:+$MYVAR:}standardflavorpath; ./A.sh
MYVAR: abc:standardflavorpath
ben@skylab:~/tmp> export MYVAR=${MYVAR:+$MYVAR:}standardflavorpath; ./A.sh
MYVAR: abc:standardflavorpath:standardflavorpath
skriesch commented 2 years ago

I tried in the spec file exactly, what will happen, if I want to add an "export PYTHONPATH" in the spec file. The last case has happened there. Therefore, I followed the results, that it has got a dependency to the python rpm macros, where it will be set with: local intro = "%{python_expand PYTHONPATH=${PYTHONPATH:+$PYTHONPATH:}"

I wanted to find a way to provide PYTHONPATH in a correct and easy way then and that would be upstream. If I receive the PYTHONPATH double within the export, then one of them comes from here,

mcepl commented 2 years ago

@bnavigator :

  1. Yes, double entry could be a problem (/etc/profile in Fedora/RHEL has some special shell functions for it, but it really feels like an overkill to me), but I don’t think it is that big a deal. Two calls of %pytest macro are extremely rare (22 out of 14325 SPEC files in Factory) and in the end it doesn't hurt most calls.
  2. What exactly do you see as a mistake in this PR?
bnavigator commented 2 years ago
  1. Yes, double entry could be a problem

That's not the point. The point is that entries are added in each flavor iteration, but not removed. You will end up with a PYTHONPATH looking into all flavor sitelibs at the last iteration, with the first one as priority:

PYTHONPATH=localsitelib
%python_expand export PYTHONPATH=${PYTHONPATH:+$PYTHONPATH:}%{buildroot}%{%python_sitelib}

results in this after 3 flavors:

PYTHONPATH=localsitelib:%{buildroot}%{python39_sitelib}:%{buildroot}%{python310_sitelib}:%{buildroot}%{python38_sitelib}
  1. What exactly do you see as a mistake in this PR?
  1. The above accumulation of different flavors in PYTHONPATH
  2. This PR changes
PYTHONPATH=$something  PYTHONDONTWRITEBYTECODE=1 pytest-3.8 $pytestargs

to

export PYTHONPATH=$something PYTHONDONTWRITEBYTECODE=1 pytest3.8 $pytestargs

Which causes the the shell to try to export a lot, but not execute pytest at all. In fact it would yield an error, because pytest-3.8 is not a valid variable name:

$ export MYVAR=abc pytest-3.8
export: not valid in this context: pytest-3.8
mcepl commented 2 years ago

That's not the point. The point is that entries are added in each flavor iteration, but not removed. You will end up with a PYTHONPATH looking into all flavor sitelibs at the last iteration, with the first one as priority:

Right, that is a problem.

mcepl commented 2 years ago

I think this whole PR is misguided, because of misunderstanding in #124. We won’t be introducing export here.