-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Shared: Remove the language-specific model generator scripts #19452
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.py
to handle all languages and add a command‑line entry point. - Delete each
GenerateFlowModel.py
underrust/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--language
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This PR
--language
flag that specifies which language to generate models for.generate_flow_model.py
togenerate_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?