How will we update pylint? about actions-ci-circuitpython-libs HOT 26 CLOSED

I think the idea was to just have one place to do it rather than dig into lots of repos. Why do you think it needs to be done piecemeal?

from actions-ci-circuitpython-libs.

If the individual repos require changes from switch from 1 to 2 then you won't be able to it. If the changes for are backwards compatible you can do it but then it isn't enforced until this version is flipped.

from actions-ci-circuitpython-libs.

I think if we need any to stay on an older version, we could set an environment variable in the Github Actions workflow for that page and add a check in this script.

from actions-ci-circuitpython-libs.

This is a good discussion point. My half-considered medium term plans for actions on the libs is to add some hooks to the workflow to allow libs to override the defaults. I'm going to think deeper about this when I work on my CP 2020 post tonight

from actions-ci-circuitpython-libs.

Sounds good! Thank you both.

from actions-ci-circuitpython-libs.

Any more thoughts? I don't think pylint 1.9.2 works on Python 3.7 so I'd like us to start using newer pylint soon.

from actions-ci-circuitpython-libs.

I haven't had any more substantial thoughts on this directly but moving to pylint 2.x is high on @dherrada 's list, I believe just after a super secret project that he hasn't been told about yet. This will have to be solved as part of that. I'll try and nail down some specifics with @sommersoft before we get to that point

from actions-ci-circuitpython-libs.

No more thoughts on this from me either. I still think adding the ability to set something in the Github Actions workflow to a specific version of pylint to override the default is the right approach and then just set the default here.

from actions-ci-circuitpython-libs.

Ok, perfect. We'll likely want a way to move individual repos at a time. I'm pretty sure I've found a pylint bug so making it easy for us to update would be great. That way I can fix it upstream and then update. :-) Thanks!

from actions-ci-circuitpython-libs.

FYI - on my Raspberry Pi -- I have updated Pylint and it still seems to be working OK with the library I am working on (RFM9x)

from actions-ci-circuitpython-libs.

@siddacious 🕵️‍♂️

from actions-ci-circuitpython-libs.

Ok, I did have some more thoughts on this. I usually run a newer version of pylint locally and I usually have to ignore some of the checks that are failing because they don't fail when uploaded. This means that if we update to a newer version, some repos that were previously passing may fail and it may mean updating the .pylintrc file on those to ignore checks that we aren't concerned about. The cookie cutter will probably need to be updated soon too. This is good in the long run because it will show more consistently between running locally and remotely, but may be a bit annoying until we get there in the short-term.

from actions-ci-circuitpython-libs.

@kattni and I had a small discussion concerning this, and I know that there are internal discussions on-going. But, I wanted to offer an approach that I came up with.

• Process:

  • Adabot patch all .pylintrc files with the following comment at the top: # pylint_version: 1.9.2

  • Update the Install Dependancies script to parse the pylint_version from .pylintrc, and pip install the respective version. This will fallback to a default of the current version 2.4.4 if the version is not parsed. I have a proof-of-concept:

    • Install deps changes here: https://github.com/sommersoft/actions-ci-circuitpython-libs/blob/pick_a_pylint/install.sh
    • Here are some test runs (failures on purpose so I can re-run them at will). See the Install deps log; debug info is printed on the first few lines, and the final pip results are near the end.
      • Install 1.9.2 explicitly: https://github.com/sommersoft/actions_testing/runs/444714583?check_suite_focus=true
      • Install fallback of 2.4.4 (version number removed from # pylint_version:): https://github.com/sommersoft/actions_testing/runs/444723888?check_suite_focus=true

  • As a library is brought up to a new pylint version, update or remove the pylint_version comment from .pylintrc.

• Advantages:

  • Visibility of progress. Adabot can check the pylint_version of each library by peeking at the .pylintrc file, and report accordingly. This would provide a daily list of libraries needing to be updated on circuitpython.org/contributing.
  • Limited shuffling of the Actions workflows on each library. Any future updates are also confined to the install.sh script.
  • The .pylintrc file will ostensibly stay the same with the version updates, but if it does need changes those changes can most likely be applied at the same time as the pylint_version patch. (There may be a few unique .pylintrcs out there…)

• Disadvantages:

  • There is a bit of complexity with this approach.
  • Requires each library to be touched at minimum to update the pylint_version. (Although, it could likely be scripted.)
  • If .pylintrc is mistakenly not updated with pylint upgrading, the old version is still pinned.

Butcher this idea at will. It was kind of off-the-cuff, and as I'm wont to do, it focuses on Adabot a lot. 😄 🤖

from actions-ci-circuitpython-libs.

I like it. Having adabot doing all the work makes it an easy implementation. :)

from actions-ci-circuitpython-libs.

from actions-ci-circuitpython-libs.

@sommersoft I like that idea! What about only including the major version such as 1 or 2? I don't think we need to peg exact version.

from actions-ci-circuitpython-libs.

What about only including the major version such as 1 or 2? I don't think we need to peg exact version.

I actually didn't know if pip supported this. Looks like it its possible. Easiest seems to be pip install pylint~=x.0.

(.venv) ~/Dev/test:$>  pip install 'pylint~=2.0'
Collecting pylint~=2.0
........
Successfully installed pylint-2.4.4

(.venv) ~/Dev/test:$> pip install --force-reinstall 'pylint~=1.0'
Collecting pylint~=1.0
........
Successfully installed pylint-1.9.5

With regard to only pinning to major versions, I will note that reading through pylint's changelog they do add new checks in minor versions.

from actions-ci-circuitpython-libs.

I've done pip install "pylint<3"

from actions-ci-circuitpython-libs.

After more thought on this, could we just move the pylint, black and sphinx installs back into build.yaml? Having them hidden behind install.sh makes it non-obvious what install locally when fixing an issue. It's a very simple way to solve the versioning issue as well.

from actions-ci-circuitpython-libs.

I agree with moving back to build.yml. I couldn't find the version info when looking for it. As well, it will allow for running different versions of Pylint on different libraries as we move through upgrading to the latest Pylint.

I also agree that pinning it to anything may not make sense as new checks are added with minor releases. Regardless, this will force us to keep up with updating our code on a more regular basis versus this sort of major push again after avoiding it for too long.

Please move the Pylint, Black, and Sphinx installs back into the build.yml file.

from actions-ci-circuitpython-libs.

Ok. New plan of attack:

• Draft changes to .github/workflows/build.yml in Adafruit_CircuitPyton_TestRepo

Once draft is approved:

• Submit PR to update circuitpython-adafruit-cookiecutter's version.
• Submit PR for git patch file to adabot/patches.
• Once patch file is merged, patch the planet! 🌎
• Submit PR to remove pip installs for pylint and Sphinx+theme in actions-ci-circuitpython/install.sh

from actions-ci-circuitpython-libs.

@sommersoft Step 1 complete and approved. Let's move on to the next steps. Thanks!

from actions-ci-circuitpython-libs.

Patches applied. Results:

.... Beginning Patch Updates ....
.... Working directory: /home/sommersoft/Dev/adabot/adabot
.... Library directory: /home/sommersoft/Dev/adabot/adabot/.libraries/
.... Patches directory: /home/sommersoft/Dev/adabot/adabot/patches/
.... Deleting any previously cloned libraries
.... Running Patch Checks On 225 Repos ....
   . Skipping Adafruit_CircuitPython_VS1053: patch does not apply
   . Skipping Adafruit_CircuitPython_BLE_Magic_Light: patch does not apply
   . Skipping Adafruit_CircuitPython_Display_Notification: patch does not apply
   . Skipping Adafruit_CircuitPython_BLE_Apple_Notification_Center: patch does not apply
   . Skipping Adafruit_CircuitPython_BLE_BroadcastNet: patch does not apply
   . Skipping Adafruit_CircuitPython_TestRepo: patch does not apply
   . Skipping Adafruit_CircuitPython_Bundle: patch does not apply
.... Patch Updates Completed ....
.... Patches Applied: 215
.... Patches Skipped: 7
.... Patches Failed: 3 

.... Patch Check Failure Report ....
No Failures


.... Patch Apply Failure Report ....
>> Repo: Adafruit_CircuitPython_Display_BLE_Status      Patch: 0001-build.yml-move-pylint-black-and-Sphinx-installs-to-e.patch
   Error: remote: Permission to adafruit/Adafruit_CircuitPython_Display_BLE_Status.git denied to sommersoft. The requested URL returned error: 403

>> Repo: Adafruit_CircuitPython_BLE_Eddystone   Patch: 0001-build.yml-move-pylint-black-and-Sphinx-installs-to-e.patch
   Error: remote: Permission to adafruit/Adafruit_CircuitPython_BLE_Eddystone.git denied to sommersoft. The requested URL returned error: 403

>> Repo: Adafruit_CircuitPython_NeoPixel        Patch: 0001-build.yml-move-pylint-black-and-Sphinx-installs-to-e.patch
   Error: remote: Permission to adafruit/Adafruit_CircuitPython_NeoPixel.git denied to sommersoft. The requested URL returned error: 403

I'll handle the 8 skips/failures manually (if necessary).

from actions-ci-circuitpython-libs.

@sommersoft Thank you!

from actions-ci-circuitpython-libs.

No need to address:

.... Patch Apply Failure Report ....
>> Repo: Adafruit_CircuitPython_Display_BLE_Status 

The repo has been archived. 😄

from actions-ci-circuitpython-libs.

Once I patch these last 9 repos, I'll close this...

Excellent work, communication, and perseverance to everyone!!

from actions-ci-circuitpython-libs.