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

Create separate automodel pack #13879

Merged
merged 12 commits into from
Aug 30, 2023
2 changes: 2 additions & 0 deletions codeql-workspace.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ provide:
- "*/ql/test/qlpack.yml"
- "*/ql/examples/qlpack.yml"
- "*/ql/consistency-queries/qlpack.yml"
- "*/ql/automodel/src/qlpack.yml"
- "*/ql/automodel/test/qlpack.yml"
- "shared/*/qlpack.yml"
- "cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/qlpack.yml"
- "go/ql/config/legacy-support/qlpack.yml"
Expand Down
28 changes: 28 additions & 0 deletions java/ql/automodel/publish.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#!/bin/sh
set -e

AUTOMODEL_ROOT="$(dirname $0)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that the original script has this same line (which I wrote a couple of years ago), but not sure if it's the best. This script is only running successfully from the codeql/java/ql/automodel directory. Running in the repo root is failing for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah...I think the path in line 5 needs to be normalized. Running from the repo root will make WORKSPACE_ROOT=java/ql/automodel/../../.., but it really needs to be WORKSPACE_ROOT=..

Suggested change
AUTOMODEL_ROOT="$(dirname $0)"
AUTOMODEL_ROOT="$(realpath "$(dirname $0)")"

Copy link
Contributor Author

@starcke starcke Aug 7, 2023

Choose a reason for hiding this comment

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

I have changed this to do:

AUTOMODEL_ROOT="$(readlink -f "$(dirname $0)")"
WORKSPACE_ROOT="$AUTOMODEL_ROOT/../../../.."

Happy to switch to realpath as well if that is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also updated the solorigate script with this change.

WORKSPACE_ROOT="$AUTOMODEL_ROOT/../../../.."
Copy link
Contributor

@aeisenberg aeisenberg Aug 4, 2023

Choose a reason for hiding this comment

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

Suggested change
WORKSPACE_ROOT="$AUTOMODEL_ROOT/../../../.."
WORKSPACE_ROOT="$(realpath "$AUTOMODEL_ROOT/../../..")"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have always been using readlink -f do you know if realpath is ok on all the places this could be thought to run?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. I've usually just used realpath in situations like this. If readlnk works, too then that's probably fine.

GRPS="automodel,-test"

if [ -z "$CODEQL_DIST" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

(Optional)...you could also check if gh codeql is available and use that if CODEQL_DIST is unset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets defer that in case we want to make changes to the solorigate pack as well.

echo "CODEQL_DIST not set"
exit -1
fi

cd "$AUTOMODEL_ROOT"
echo Testing automodel queries
"${CODEQL_DIST}/codeql" test run test

cd "$WORKSPACE_ROOT"

echo Preparing release
"${CODEQL_DIST}/codeql" pack release --groups $GRPS

echo Publishing automodel
"${CODEQL_DIST}/codeql" pack publish --groups $GRPS

echo Bumping versions
"${CODEQL_DIST}/codeql" pack post-release --groups $GRPS

echo Automodel packs successfully published. Please commit and push the version changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you wanted to be extra fancy, you could commit and push directly from this script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I kind of like it not committing for me, then there is less that suddenly happens. But we can also tweak this afterwards. It almost seems like there should be a generic publish script we could have for all the separate packs, but I am not sure how frequent these separate packs are?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this script is going to be run in CI, then something will have to commit and push the change, but maybe that is a longer term goal.

Eventually, all of our core packs will be publishable separately. But, that won't happen for a while. I still would hope that something like this script would become the default way of publishing.

8 changes: 8 additions & 0 deletions java/ql/automodel/src/qlpack.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
name: codeql/java-automodel-queries
version: 0.0.1-dev
groups:
- java
- automodel
dependencies:
codeql/java-all: ${workspace}
warnOnImplicitThis: true
12 changes: 12 additions & 0 deletions java/ql/automodel/test/qlpack.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
name: codeql/java-automodel-tests
version: 0.0.1-dev
groups:
- java
- automodel
- test
dependencies:
codeql/java-all: ${workspace}
codeql/java-automodel-queries: ${workspace}
extractor: java
tests: .
warnOnImplicitThis: true
Loading