[bug#53402] Rebase it for the new python packages

Hello, and thank you for your new review!

Le dimanche 06 février 2022 à 20:06 +0000, Vinicius Monego a écrit :
> 
> I noticed that some of the tests aren't running, like in flake8-array-
> spacing. If the check phase ends with "Ran 0 tests" then the tests are
> not being collected.


> If there are no tests to be collected, the package should have a
> #:tests? #f along with a comment saying that there are no tests. If
> there are tests to run, the check phase will have to be overriden to
> run them.

I tried to make sure that this situation didn’t happen again.

> > For MNE, the test data set is a separate repository without a
> > license,
> > so I disabled the tests.
> > 
> 
> OK. I tried to download the test dataset from within the mne module
> and
> they don't have a license agreement or anything, while to download
> individiual datasets the user has to agree to the (non-free) terms. I
> wonder if that's acceptable for merging in Guix?

I think it’s not, plus these are medical data (anonymized but still) so
maybe we shouldn’t take the risk.

> In [1] I found that there are more base dependencies that aren't
> listed
> in the pypi importer. They should be added to propagated-inputs. If
> tests can't run, then native-inputs can be removed.

I had nervous laughter when I read that page. I managed to package
everything except for the JS widgets (I don’t know how to tackle that
and I fear it would be a lot of work for very little benefit, since
there are other visualization platforms from what I understand) and
mne-qt-browser, because it depends on MNE.

> Usually, pytest modules should go into python-check.scm, not python-
> xyz.scm.

I tried to separate them, but they depend on a lot of stuff which is
not easy to organize into acyclic module imports.

> > > Could you style it as
> > > 
> > >     (source
> > >      (origin
> > >        (method url-fetch)
> > >        (uri (pypi-uri "imageio-ffmpeg" version))
> > >        (sha256
> > >         (base32
> > > "0ff14079izsyxwf6ki68k9a7w5krjlal7lwqvzg2bbddl92l5spj"))))
> > > 
> > > and the other packages too?
> > 
> > OK.
> > 
> 
> The base32 line was truncated in the mail, the hash should be in the
> same line of 'base32'. But I can fix that.

I fixed that occurence, but as a general rule I’m not very confident in
my indentation taste.

> 
> [...]
> 
> > 
> > > 
> > > Some of the descriptions are not full sentences (e.g. in python-
> > > pytest-
> > > harvest-minimal). Please check that descriptions are full
> > > sentences.
> > 
> > I’m not sure I understand. I reworked some descriptions, but didn’t
> > find non-full sentences. Could you explain what you mean?
> > 
> 
> Full sentences are made of a subject + predicate. This one:
> 
> +    (description "I/O support for EEGLAB files in Python.")
> 
> doesn't have a subject.
> 
> Usually the subject in the description is the package's name itself
> or
> "This package...". e.g. "EEGLABIO is a library..." or "This package
> provides I/O support..."

I think I got it.

> 
> Something else to avoid in descriptions is marketing talk, such as
> 'simple and reliable' in python-imageio-ffmpeg.

There were some more occurences that I neutralized.

> 
> [...]
> 
> The package modules you changed are also missing your copyright line.

OK.

As you see in the new series, vtk doesn’t install an egg-info, which
breaks the sanity-check phase of dependent python packages. There was
an option to let setuptools build the whole project, so the egg-info
would be installed too, but the installation plan with setup.py is
dysfunctional. So I made a terrible hack to "install" an egg-info.
There’s surely a better way to do it with python or pip, but I have no
clue as to what it would be and I can’t do much trial and error since I
have to wait for everything to build again (and vtk is quite a long
thing to compile).

Here is the v5!

I’m not used to managing such a large patch series; I hope I didn’t
make too many errors.

Best regards,

Vivien

> 
> [1]
> https://github.com/mne-tools/mne-python/blob/main/requirements_base.txt

Hello!

Vinicius, could you take a look at v5 of this patch series?

Thanks for reviewing!

Ludo’.

On 06/03/2022 6:48 PM, Ludovic Courtès wrote:

> Hello!
>
> Vinicius, could you take a look at v5 of this patch series?
>
> Thanks for reviewing!
>
> Ludo’.

Hi Ludo, Vivien.

Sorry for the long wait. I will be busy until mid-April. I did review
the patches earlier and LGTM apart from minor style/lint and description
issues, but I could fix that before committing. I saw that someone
pushed a wip-python-mne branch and thought they were taking care of this.

I have some questions for other maintainers, like

1. Is it OK to have that many bootstrap packages for pytest modules
(patches 8 to 16) and in files different than where they are supposed to
be (e.g. decopatch-minimal in python-check to bootstrap pytest-harvest)?
It seems that there are no other options in this case though.

2. Should python-no-verison (patch 22) be a public variable? It is an
example package, doesn't seem to be useful to expose it.

If it's OK I could push the series up to patch 26. I haven't reviewed 27
and on in v5.

3. Not a question but it would be good to review FSDG status of
python-mne, since it contains code to download non-free data.