Shared: Remove the language-specific model generator scripts #19452
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates the per-language flow model generator scripts into a single shared generate_mad.py tool with a new --language flag, and removes the language‑specific stubs.
- Refactor
misc/scripts/models-as-data/generate_mad.pyto handle all languages and add a command‑line entry point. - Delete each
GenerateFlowModel.pyunderrust/ql,java/ql,csharp/ql, andcpp/ql. - Update
RegenerateModels.py(Java) and the CI workflow to invoke the new script.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/src/utils/modelgenerator/GenerateFlowModel.py | Removed Rust‑specific generator stub. |
| misc/scripts/models-as-data/generate_mad.py | Refactored to support --language, cleaned imports, added __main__. |
| java/ql/src/utils/modelgenerator/RegenerateModels.py | Updated to call generate_mad.py with --language java. |
| java/ql/src/utils/modelgenerator/GenerateFlowModel.py | Removed Java‑specific generator stub. |
| java/ql/src/change-notes/2025-05-02-mad-generator-renamed.md | Documented removal of old script and new --language requirement. |
| csharp/ql/src/utils/modelgenerator/GenerateFlowModel.py | Removed C#‑specific generator stub. |
| csharp/ql/src/change-notes/2025-05-02-mad-generator-renamed.md | Documented removal of old script and new --language requirement. |
| cpp/ql/src/utils/modelgenerator/GenerateFlowModel.py | Removed C++‑specific generator stub. |
| .github/workflows/mad_modelDiff.yml | Updated CI to invoke generate_mad.py with --language java. |
Comments suppressed due to low confidence (1)
misc/scripts/models-as-data/generate_mad.py:81
- The
sys.exit(0)in the language‑not‑specified branch is mis‑indented (it ends up running unconditionally) and exits with a success code. It should be indented inside theelse:block alongside theprint(), and use a non‑zero exit code (e.g.sys.exit(1)), so that missing--languageis treated as an error.
sys.exit(0)
| @@ -68,7 +68,7 @@ jobs: | ||
| DATABASE=$2 | ||
| cd codeql-$QL_VARIANT | ||
| SHORTNAME=`basename $DATABASE` | ||
| python java/ql/src/utils/modelgenerator/GenerateFlowModel.py --with-summaries --with-sinks $DATABASE $SHORTNAME/$QL_VARIANT | ||
| python misc/scripts/models-as-data/generate_mad.py --language java --with-summaries --with-sinks $DATABASE $SHORTNAME/$QL_VARIANT |
There was a problem hiding this comment.
The CI invocation omits the --with-neutrals flag, but RegenerateModels.py includes it. Add --with-neutrals here to ensure neutral models are generated and compared in the diff job.
| python misc/scripts/models-as-data/generate_mad.py --language java --with-summaries --with-sinks $DATABASE $SHORTNAME/$QL_VARIANT | |
| python misc/scripts/models-as-data/generate_mad.py --language java --with-summaries --with-sinks --with-neutrals $DATABASE $SHORTNAME/$QL_VARIANT |
Copilot uses AI. Check for mistakes.
Co-authored-by: Michael Nebel <michaelnebel@github.com>
|
Thanks for the review :) I've applied the suggestions. |
| @@ -0,0 +1,8 @@ | ||
| --- |
There was a problem hiding this comment.
Considering https://github.slack.com/archives/C06016CLLGY/p1747081761540249
then we should probably not have a change note for this PR.
There was a problem hiding this comment.
Makes sense and done. It's honestly also a bit easier, if we can consider the model generator internal.
There was a problem hiding this comment.
Yes, I agree - but I think that the field services team uses the model generator queries - so we might break stuff, if we make changes to the queries (however, the script is to my knowledge not used elsewhere)
Excellent, I have added one extra ask 😄 |
This PR
--languageflag that specifies which language to generate models for.generate_flow_model.pytogenerate_mad.py. Seems like a slightly nicer name as the script can also generate sinks and sources.I think this will make it easier to expand on the script in a cross-language way going forward.
The model difference CI job is failing. I think that's because it's trying to use the new script path on the old main, which can't work. I think we can just ignore that?