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

Initial TrustInSoft CI configuration #1003

Closed
wants to merge 1 commit into from
Closed

Initial TrustInSoft CI configuration #1003

wants to merge 1 commit into from

Conversation

guillaumemillot
Copy link

@guillaumemillot guillaumemillot commented Oct 13, 2020

This PR adds an initial TIS CI configuration to libsodium:
https://ci.trust-in-soft.com/projects/guillaumemillot/libsodium/1

The configuration includes 56 tests from the /test/default folder and covers the single architecture x86-64 to get started.
Note the 5 tests in red which raise an alarm due to address comparisons. We'll soon add an option to ignore it.

I'm asking you a few questions related to this configuration in issue #995. I would appreciate your feedback!

About the files that are part of this PR:

  • tis.config:
    The main configuration file that describes the 56 tests to analyze. Each entry in this JSON corresponds to one test.
    (For more information on configuration files: https://docs.ci.trust-in-soft.com/reference/configuration-file)
  • trustinsoft/common.config and trustinsoft/gcc_x86_64.config:
    These are small configuration files that contain analysis options either common to all tests or specific to the x86-64 architecture. The files are included in the tis.config file via the option include.
  • trustinsoft/stub.c:
    This file provides stubs to some functions that are not available on TIS CI.
  • test/default/*.c:
    7 tests have been modified for TIS CI to make them quicker to analyze. I would appreciate your feedback to tell if the tests are still relevant!
  • README.md:
    Includes a TIS CI badge that displays the latest analysis status for master.

Pending your review, I expect the PR to be mergeable as is. You are free to merge it on a test branch first or on master directly.

@jedisct1
Copy link
Owner

Thanks a lot for this!

Having TIS integrated in our CI will be very valuable.

However, that configuration makes me freak out. It will have to be updated every time a file is added, moved, renamed, or deleted. Or even when something is modified and it doesn't match the expected TIS environment.

It's very likely that updating that configuration will constantly be forgotten.

It also requires a (modified?) copy of an automatically generated file.

These introduces additional burden, especially compared to other static analyzers that don't require such a configuration. The more of these burden we add, the more discouraging it is to work on the project.

We currently have a script (regen-msvc/regen-msvc.py) that creates the Visual Studio solutions according to the current files structure.

Could that script (or another one, with both that could be run with a single command) be used to generate the TIS configuration simultaneously?

Could the predefined macros be produced by a ./configure output (probably with a specific target) instead of being hardcoded?

@jakub-zwolakowski
Copy link

jakub-zwolakowski commented Oct 23, 2020

Hi again @jedisct1 !

Thanks for your answer. I definitely understand your concerns... This kind of feedback is actually very valuable for us, as we are figuring the question of making robust TIS CI configurations at this moment (until now we mostly deployed TIS CI on projects where very little change was expected).

So I thought about the issues you have mentioned and tried to improve the TIS CI configuration. I have adjusted the config files a bit (the analyses take a little longer now, but the configuration is definitely more robust) and I have written a python script which:

  • regenerates all the config files (tis.config, trustinsoft/common.config, trustinsoft/gcc_x86_64.config),
  • copies all the automatically generated files which need to be committed to the repo (i.e. just src/libsodium/include/sodium/version.h at this point) to the right places.

I hope this script will be good enough for you as a (partial) solution of these issues.

Unfortunately I don't have a full automatic solution for all the possible cases "when something is modified and it doesn't match the expected TIS environment" yet. What I can propose is that if any time soon your TIS CI config is broken, and running the regen-trustinsoft.py is not enough to repair it, we (i.e. TIS CI team) are going to step in and handle it. This will be a perfect opportunity for us to understand better what kind of changes we have to take into account in order to make the TIS CI configurations robust and / or what tools we need to develop in order to automatically adapt these configurations.

Is that approach OK for you?

I'll make a PR with the new (more robust) config and the regen-trustinsoft.py script shortly.

@jedisct1 jedisct1 closed this Nov 2, 2020
Repository owner locked and limited conversation to collaborators Feb 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants