Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nit: Fix runtime dependent format issue in generate.sh and clean up python scripts #641

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

crest42
Copy link

@crest42 crest42 commented Feb 18, 2025

Hey!

I was debugging something related to code generation and it seems that every time I ran the generate.sh script, a ton of local files were changed due to formatting.

This PR contains two changes:

  1. Add --style="{BasedOnStyle: llvm, IndentWidth: 4}" to clang-format in generate.sh

In my environment the generate.sh script's clang-format execution changes every file to fix whitespaces (which I assume is somewhat my Arch Linux's clang-format default). By using the same settings as in code_format.sh we can make sure that the changes are somewhat consistent and that we don't end up with a mess of changes files.

I also removed a committed .clang-format file which is inconsistent with the current code formatting and the config in do_check_format.sh and generate.sh

It also seems like since we do a find on '.' (and from the .gitignore it seems that I am not the only one) every .c .h and .inc file in the directory gets formatted and therefore the checked out versions of liboqs and openssl get formatted as well. This takes a very long time and IMHO it does not make too much sense formatting those repos. I limited the find command in the generate.sh script to format only oqsprov/ and test/

  1. Nitpicky cleanup with generate.py and generate_oid_nid_table.py

Some Python best practices have been violated and I took the freedom to fix what I deem risk-free. I can remove those changes if they are not wanted since it shouldn't change any behavior. the changes in detail:

  • Unify whitespace to 4 spaces (been mixed)
  • Use main guard in generate.sh and generate_oid_nid_table.py
  • Use a main function as the entry point for generate_oid_nid_table.py instead of just importing the file
  • Fix possible undefined variable runtime error and throw an exception instead in generate_oid_nid_table.py
  • Use f-strings for better readability
  • Rename variable to avoid shadowing of outer scopes and reduce ambiguity
  • Use reference check (is None) instead of equality checks (== None)
  • Explicitly do not check for rc == 0 in subprocess.run by setting check=False
  • Explicitly set encoding on calls to open(...)

@crest42 crest42 requested a review from baentsch as a code owner February 18, 2025 14:09
@crest42
Copy link
Author

crest42 commented Feb 18, 2025

I hope those changes are helpful. If not feel free to just close the PR.

There are still two open issues:

  1. The changes I did to generate.sh might just indicate that this file is not used and people use the Python script directly? If yes I can also git rm it.

  2. The formatting still seems to change files in oqsprov:

     modified:   examples/static_oqsprovider.c
     modified:   oqsprov/oqsdecoders.inc
     modified:   oqsprov/oqsencoders.inc
     modified:   oqsprov/oqsprov.c
     modified:   oqsprov/oqsprov_capabilities.c
    

Those files are also changed by do_code_format.sh. While looking into it there seem to be more inconsistencies:

  • generate.sh also takes .inc files into account while do_code_format is limited to .c and .h files
  • It might just be better to call do_code_format from generate.sh so we can avoid having to change both files all the time? But then we would miss formatting the .inc files so I'd either have to update the do_code_format script

@crest42
Copy link
Author

crest42 commented Feb 18, 2025

My final suggestion would be:

  • Make generate.sh call do_code_format.sh explicitly instead of duplicating functionality. Optional: remove generate.sh
  • Ignore .inc files in formatting since they are auto-generated anyways.
  • Align oqsprov/oqsprov.c and oqsprov/oqsprov_capabilities.c with correct formatting
  • Add format check to CI pipeline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant