Codecov Report

Merging #2486 (90a2029) into release/8.2 (93d41fa) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           release/8.2    #2486      +/-   ##
===============================================
- Coverage        46.45%   46.45%   -0.01%     
===============================================
  Files              526      526              
  Lines           119261   119260       -1     
===============================================
- Hits             55404    55403       -1     
  Misses           63857    63857              

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Some(several?) updates must be cherry picked into release/8.2 , i.e. #2433

Some(several?) updates must be cherry picked into release/8.2 , i.e. #2433

My git cherry-pick workflow does not seem well founded for this project. And there are 30 py files that black changes.

Some(several?) updates must be cherry picked into release/8.2 , i.e. #2433

My git cherry-pick workflow does not seem well founded for this project. And there are 30 py files that black changes.

I usually create a branch from release/8.2 and cherry-pick the commits from master that are related to the CI. But yeah, a bit of manual effort is in order

@nrnhines : did you check the master and see if there are other commits that we need to back port?

did you check the master

Yes. This PR has the substantive user fixes. Of course I'm willing to entertain others I might have missed. But I'm having trouble with the minimal build and CI related changes needed for 8.2.3 to pass and build the wheels and setup.exe.
d8c113a above had 5 conflicts. I fear that one setup.py to rule them all (#2235) needs to be cherry picked first but it has many conflicts as well. I worry that a few things about it are silently accepted and if I choose to resolve a conflict in favor of old 8.2.2, I'll introduce a bug. I am playing with a temporary PR #2487 that focuses just on the build issue but can't yet succeed with a windows build.

And there are 30 py files that black changes.

Out of curiosity, does anyone understand how this can be? black was run on everything, and no new changes are allowed unless they're unchanged by running black

✔️ 2e86f71 -> Azure artifacts URL

✔️ 7fb42f5 -> Azure artifacts URL

@pramodk

Disable shallow clone with the option

What was the rationale for that?

What was the rationale for that?

I didn't spend much time but the CI [failure here was](https://github.com/neuronsimulator/nrn/actions/runs/6114834861/job/16597216261
https://tracking.duperrex.ch/2p8refhd):

fatal: No names found, cannot describe anything.

Hence tried to do a full clone. (Didn't check if there is any other change in the config somewhere in current master. cc: @heerener)

@ramcdougal

And there are 30 py files that black changes.

Out of curiosity, does anyone understand how this can be? black was run on everything, and no new changes are allowed unless they're unchanged by running black

I don't have a story yet, but here are some observations.

black share/lib/python/neuron/doc.py exhibits minor reformatting for both master and 8.2.2

8.2.2 .github/workflows/formatting.yml runs black on all respository py,ipynb files with

        run: |
          git ls-tree -z -r HEAD --name-only \
          | grep -z '\.\(py\|ipynb\)$' \
          | xargs -0 black --check --color --diff --target-version py37

whereas the master version of this file uses

run: external/coding-conventions/bin/format -v --dry-run

Running the latter manually does not change any files in master (coding-conventions HEAD detached at a377933) and on examining stdout/stderr, the black line that mentions doc.py

external/coding-conventions/bin/format -v --dry-run >& temp
...
/home/hines/neuron/temp/.bbp-project-venv/bin/black --color --target-version py37 --check --diff --color share/lib/python/neuron/psection.py ...  share/lib/python/neuron/doc.py ...
All done!
30 files would be left unchanged.

Just for fun, I tried running the last also in 8.2.2 (note that coding-conventions for master is a half dozen commits ahead of what
8.2.2 gets (5d4bcd2d) and I had to $ cp external/coding-conventions/.bbp-project.yaml .)
There were many errors about clang-format, but

/home/hines/neuron/8.2.3/.bbp-project-venv/bin/black ... share/lib/python/neuron/doc.py ...
error: cannot format share/lib/python/neuron/rxd/geometry3d/graphicsPrimitives.pyx: Cannot parse: 9:8: cimport cython
Oh no!
29 files would be left unchanged, 1 file would fail to reformat.

And at the end there are no modified files.

I wonder if, in fact, the master CI black check never fails.

It used to fail, because I've had to go back and run black before.

I wonder if it's possible that different versions of black do different things.

@nrnhines : I have asked internally if someone is aware about black issue.

About the documentation failure, I see:

In #2379, Alex has already documented the following in this file:

## Hacking the Python Domain

 It is sometimes required to re-hack the domain to make it work with the latest version of Sphinx. 
 To that end, the following script can be used to generate the domain from the Python domain:

 ```bash
 cd docs
 python3 generate_hocdomain.py
 

 This script generates a HOC domain from the one available in the sphinx package and writes it to:

     docs/domains/hocdomain.py

 A comment is added at the top of the file to indicate that it is a generated file and the Sphinx version used to generate it.

I see Running Sphinx v7.2.5 in documentation CI. So let me take docs/domains/hocdomain.py from master and see what happens.

I'd say this is a matter of black evolving and getting better. As an example: in the python formatting run here we're using black 23.3.0
In the changelog for version 23.1.0, I find this item:

Remove unnecessary parentheses from tuple unpacking in for loops (#2945) (22.3.0)

(see the related black issue#2338 for an example)
That looks suspiciously like the last item black suggests in the failed workflow run here.

23.1.0 was the first version of 2023, released after what, to me, looks like the last successful run of this workflow on release/8.2: #2113 (15th of December 2022, before this particular version of black was released).

Reopening to re-trigger failing windows CI!

✔️ 22782f6 -> Azure artifacts URL

✔️ 7f811c6 -> Azure artifacts URL

thanks to @heerener's hints, we verified that the master doesn't fail because we install black version ~22.3

So one of the previous commits I downgraded the installed version. So now CI seems green (expect code coverage which should be ignored here).

now CI seems green

Excellent! When I got up this morning, I was ready to go ahead and pollute this PR with the 30 python files modified by black.
But I think this black downgrade is a better solution. Now just need an approving review :) (and all the details for tag and user facing info and urls for the installers)

I was ready to go ahead and pollute this PR with the 30 python files modified by black. But I think this black downgrade is a better solution.

Also discussed this with @heerener. I think in coming days we can change python files in master with newer Black and then backport those to 8.2 branch.

Now just need an approving review

Will do!

and all the details for tag and user facing info and urls for the installers

Before tag, we also need to update changelog updated in docs/changelog.md. Do you want to do this in this PR or a separate PR?

want to do this in this PR

I'll merge this. And start working on the changelog. Thanks!

this won't be a squash and merge

I don't understand. I was going to select squash and merge for this (and edit the commit message). Another PR will update the changelog.md . I am a bit shaky about the chicken/egg problem for updating the https://nrn.readthedocs.io/en/8.2.2/
Seemed reasonable to me to separate the substantive PR from the administrative PR.

Or is this related to

Required statuses must pass before merging

Final CI Expected — Waiting for status to be reported

I don't know how to deal with this. Hopefully, dealing with it does not require starting CI from scratch.

Final CI Expected — Waiting for status to be reported

I don't know how to deal with this. Hopefully, dealing with it does not require starting CI from scratch.

I believe this needs to go in: https://github.com/neuronsimulator/nrn/pull/2332/files

The final CI was added for convenience, otherwise for the required status one would have to manually select the jobs from the NEURON CI matrix; any change in the options of that matrix (i.e. CI job name change, adding a CMake option, ..) would require a significant manual labor to change the required statuses. Now instead of requiring that all those jobs individually, the FInal CI comes into play.

✔️ 90a2029 -> Azure artifacts URL