lint: convert submodule linter test to Python #24803

1. 
   Eunoia1729 commented at 8:27 pm on April 7, 2022: contributor

   Refs #24783
2. Eunoia1729 renamed this:
   linnt: convert submodule linter test to Python
   lint: convert submodule linter test to Python
   on Apr 7, 2022
3. Eunoia1729 force-pushed on Apr 7, 2022
4. Eunoia1729 force-pushed on Apr 7, 2022
5. DrahtBot added the label Tests on Apr 7, 2022
6. in test/lint/lint-submodule.py:18 in 1b324cf20a outdated

   13+def main():
   14+    submodule_check_args = ['git', 'submodule', 'status', '--recursive',]
   15+    try:
   16+        submodules_list = subprocess.check_output(submodule_check_args, stderr = subprocess.STDOUT)
   17+    except subprocess.CalledProcessError:
   18+        exit(1)
   

   ---

   ---

   

   KevinMusgrave commented at 10:50 am on April 8, 2022:

   I think python already returns an exit code of 1 when it raises an exception

   ---

   

   laanwj commented at 11:40 am on April 8, 2022:

   I do think it’s neater not to have a Python exception in the output, especially if the subprocess already prints its own self-evident errors.

   ---

   

   MarcoFalke commented at 11:46 am on April 8, 2022:

   This should never happen, and when it happens, the process won’t print it’s own errors, so the exception seems better?

   ---

   

   laanwj commented at 11:48 am on April 8, 2022:

   I’m pretty sure that git prints its own error when it fails. But ok, no strong opinion here.

   ---

   

   MarcoFalke commented at 11:58 am on April 8, 2022:

   check_output will eat the output. With stderr = subprocess.STDOUT it will also eat the error.

   ---

   

   laanwj commented at 5:49 pm on April 13, 2022:

   I hadn’t noticed the stderr = subprocess.STDOUT. Pretty sure we don’t need it. The original script doesn’t redirect the stderr either.

   ---

   

   Eunoia1729 commented at 5:53 pm on April 13, 2022:

   Thank you @laanwj , @KevinMusgrave and @MarcoFalke for your review and points which led me to dig further.

   I second @MarcoFalke .
   From my understanding, 2 cases are:

   1. Without stderr = subprocess.STDOUT, the error is printed onto the console and is not piped to Python.
   2. With stderr = subprocess.STDOUT, if we want to access the error message, we can access it using e.output in the exception block (where e is CalledProcessError object). Without explicit printing, error is not printed onto console.

   I’ll update the PR to print errors to keep the behaviour consistent with older version. Additionally, I don’t see any cases where this command will return exit code = 1. Should I remove try exception block ?

   Kindly let me know your views too.

   Edit: I removed try exception block.

7. 
   KevinMusgrave commented at 11:12 am on April 8, 2022: contributor

   Concept/approach ACK
8. in test/lint/lint-submodule.py:27 in 1b324cf20a outdated

   22+    if submodules_list:
   23+        print("These submodules were found, delete them:")
   24+        for submodule in submodules_list:
   25+            print(submodule)
   26+        exit(1)
   27+    exit(0)
   

   ---

   ---

   

   laanwj commented at 11:51 am on April 8, 2022:

   I do have the same nit as in the other PR #24802 (review), if you use exit, use sys.exit.

   ---

   

   Eunoia1729 commented at 7:47 pm on April 13, 2022:

   Thanks again @laanwj for the review! I’ve updated the PR.
9. Eunoia1729 force-pushed on Apr 13, 2022
10. 
    Eunoia1729 commented at 7:46 pm on April 13, 2022: contributor

    Before change:

    After change:

11. 
    KevinMusgrave commented at 7:48 pm on April 13, 2022: contributor

    Tested ACK 3289688f7e4642f0790d205dfe257cc929001b3c
12. Eunoia1729 requested review from laanwj on Apr 13, 2022
13. in test/lint/lint-submodule.py:24 in 3289688f7e outdated

    19+            '--recursive',
    20+    ]
    21+    return subprocess.check_output(command).decode('utf-8').splitlines()
    22+
    23+def main():
    24+    submodules_list = get_submodules_list()
    

    ---

    ---

    

    MarcoFalke commented at 11:15 am on April 15, 2022:

    0    submodules_list = subprocess.check_output(
    

    no need for a function, you can call the stdlib directly

    ---

    

    Eunoia1729 commented at 5:25 am on April 20, 2022:

    Sure, done !
14. in test/lint/lint-submodule.py:27 in 3289688f7e outdated

    22+
    23+def main():
    24+    submodules_list = get_submodules_list()
    25+    if submodules_list:
    26+        print("These submodules were found, delete them:")
    27+        for submodule in submodules_list:
    

    ---

    ---

    

    MarcoFalke commented at 11:16 am on April 15, 2022:

    what is the point of first splitting by newline and then joining the array by newline again when printing? Just leave it as string?

    ---

    

    Eunoia1729 commented at 5:24 am on April 20, 2022:

    I agree, that was unnecessary. Updated the PR. Thanks for pointing it out !
15. lint: convert submodule linter test to Python 4a9e36dbaf
16. Eunoia1729 force-pushed on Apr 20, 2022
17. 
    laanwj commented at 3:09 pm on April 21, 2022: member

    Tested ACK 4a9e36dbaf96f83d0829f8442114a2fa36641776

    0$ git submodule add https://github.com/bitcoin-core/HWI.git  HWI
    1$ test/lint/lint-submodule.py
    2These submodules were found, delete them:
    3  2c757c8306be929714760ddba01d8ab6b0457d99 HWI (2.1.0-rc.1-4-g2c757c8306be929714760ddba01d8ab6b0457d99)
    

18. laanwj merged this on Apr 21, 2022
19. laanwj closed this on Apr 21, 2022

    ---

20. Eunoia1729 deleted the branch on Apr 21, 2022
21. sidhujag referenced this in commit 57d8585eac on Apr 22, 2022
22. DrahtBot locked this on Apr 21, 2023

Contributors
Eunoia1729 KevinMusgrave laanwj MarcoFalke

Review Requested
laanwj

Labels
Tests