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

Add coverity push script #165

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

Conversation

paul-elliott-arm
Copy link
Member

Description

This is the script to allow us to push coverity builds from Jenkins (or anyone's machine come to that)

Functionality is documented within the script, but to test:

either:

export COVERITY_TOKEN=token
path/to/mbedtls/scripts/push_coverity_scan.py path/to/mbedtls -b origin/coverity_scan -e [email protected]

or

path/to/mbedtls/scripts/push_coverity_scan.py path/to/mbedtls -t token -b origin/coverity_scan -e [email protected]

The token can be found from the coverity web interface, and for the email address I would use your own email address for now (email address is unfortunately compulsary). Conversations are ongoing about which address to use for notifications in the future, the notification email will look like this:

 Your request for analysis of ARMmbed/mbedtls has been completed successfully.
    The results are available at <url>

    Build ID: 607763

    Analysis Summary:
       New defects found: 0
       Defects eliminated: 0

Should new defects be found, another email will be generated, but this will go to the whole team (this is specified within coverity itself)

WARNING - the mbedtls directory specified will be switched to the latest version of the branch specified, any outstanding differences will currently cause the script to fail (in my opinion better than forcing anything).

This constitutes stage 1 of getting coverity push back into CI, getting this script reviewed so that we can at least check its in the correct shape, then stage 2 will be adding the groovy and testing in the repo. I currently envisage this to be added shortly after the existing push from development to coverity_scan branches in the nightly runs.

Signed-off-by: Paul Elliott <[email protected]>
@paul-elliott-arm paul-elliott-arm added enhancement New feature or request needs: review needs: reviewer size-s Estimated task size: small (~2d) priority-high labels Apr 25, 2024
@paul-elliott-arm paul-elliott-arm self-assigned this Apr 25, 2024
@paul-elliott-arm
Copy link
Member Author

One possible improvment (although not yet required) would be to parameterise the project so that we can easily use this on other projects, however I'm not sure how many coverity scan projects we are allowed to have!

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read the code, but not tried to run it (I'll do that when I re-review). This looks good from a high-level view, but there are several places where I found the code hard to follow. Some places seem to reinvent standard library functions, and some places have difficult-to-understand error handling that looks like it could be simpler. Generally, in Python, prefer context handlers (with …) if possible.

coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
Mistaken usage of hashlib.md5(), much simpler form
is available.

Signed-off-by: Paul Elliott <[email protected]>
-j without a number means unlimited, and is likely
a bad idea on limited systems.

Signed-off-by: Paul Elliott <[email protected]>
(Rather than empty string)

Signed-off-by: Paul Elliott <[email protected]>
Signed-off-by: Paul Elliott <[email protected]>
Signed-off-by: Paul Elliott <[email protected]>
Refactor previous function as now too small to really be required (error
handling now done by exceptions thrown by run(), and only a single API
call now required)

Signed-off-by: Paul Elliott <[email protected]>
Also make branch change recurse into submodules, as we now have one.

Signed-off-by: Paul Elliott <[email protected]>
Use 'with' context, copy to backup file if required.

Signed-off-by: Paul Elliott <[email protected]>
The addition of the framework necessitates this.

Signed-off-by: Paul Elliott <[email protected]>
Missed as logic causes the script to just redownload the tools.

Signed-off-by: Paul Elliott <[email protected]>
Instead of erroring out if the specified backup directory is
non-existent, just create it.

Signed-off-by: Paul Elliott <[email protected]>
Useful for debug more than anything else.

Signed-off-by: Paul Elliott <[email protected]>
.. and update documentation to add new options.

Signed-off-by: Paul Elliott <[email protected]>
If config.h was changed upstream, it would be backed up prior to these
changes being applied locally, and then restored into the wrong state.

Signed-off-by: Paul Elliott <[email protected]>
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! On code inspection, I believe I now understand what the code is doing, and would be able to maintain it going forward.

I haven't tried running the script yet (and there's a typo that looks like it would fail midway).

I wonder how to test it: how far can we go before consuming a limited Coverity credit: is that consumed by the build request? By triggering? Could we have a test mode in the script that does everything except the credit-consuming step, and leaves behind or prints out all necessary information to do the credit-consuming step manually?

coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
coverity/push_coverity_scan.py Outdated Show resolved Hide resolved
The token can either be passed in as an argument with -t, or via the environment
in the variable 'COVERITY_TOKEN'

Other options:
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[non-blocker] It might be better to leave out the list of options here. They're already documented through the argparse calls and I fear that we'll forget to update the text here.

Build the MBed TLS source located in the passed in dir, using the tools specified, using the
given pre-build and build commands. Tar the results up into the given file name, as required by
coverity.
"""

os.chdir(mbedtls_dir)
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[non-blocker] Because of the chdir call here, I'm not sure how the each command line arguments will be interpreted if they're relative paths, or if they even can be relative paths. It would be more robust to not call chdir and pass cwd=mbedtls_dir to subprocess.run calls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really unsure on this one, the scripts themselves generally assume being run from the root MBedTLS dir, so this is how I made it, I haven't noticed any issues up till now.

Moving the backup of config files fixed the previous issue, but created
another one.

Signed-off-by: Paul Elliott <[email protected]>
Enable testing without consuming a coverity credit

Signed-off-by: Paul Elliott <[email protected]>
@paul-elliott-arm
Copy link
Member Author

I wonder how to test it: how far can we go before consuming a limited Coverity credit: is that consumed by the build request? By triggering? Could we have a test mode in the script that does everything except the credit-consuming step, and leaves behind or prints out all necessary information to do the credit-consuming step manually?

I have added -n/--no-upload which will cover this case. Should you use a backup directory for the generated tar file (rather than a temp file) this tar file can then be manually uploaded later, or you can just use this to test the tools download / build steps.

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the script locally with --no-upload and it worked, producing a plausible-looking mbedtls-24-12-06.tar.gz in the --backupdir directory. So this looks good to me both on code reading and on execution apart from possibly the upload step. I'd still like to check that the upload step works for me while you're still partly around, but not today, and in the meantime I can approve this PR already, we can take it from there.

With the addition of tf-psa-crypto as a submodule, which itself has
framework as a submodule, git submodule calls now need to be recursive

Signed-off-by: Paul Elliott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs: work priority-high size-s Estimated task size: small (~2d)
Projects
Development

Successfully merging this pull request may close these issues.

2 participants