Add scripts to automate dependencies #2313

jaisnan · 2023-03-23T01:39:52Z

Description of changes:

These scripts help automate part of the dependency scripts. While initially tested for manual use, it can be developed and used as part of the CD.

Call-outs:

These scripts only automate the part of the deployment that deal with updating dependencies and updating version numbers. If approved, the rest of the deployment can be added easily (the PR for that is ready to be added).

Testing:

How is this change tested? Manually
Is this a refactor change? No

Checklist

Each commit message has a non-empty body, explaining why the change was made
Methods or procedures are documented
Regression or unit tests are included, or existing tests cover the modified code
My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

karkhaz

Apart from all the specific comments:

Please run these files through a linter like pylint or pep-8
Would you be able to write a top-level script that runs all of these? It's not clear to me how they should be run, or on what order. Some of my comments suggest that the small update-$TOOL.py duplicate a lot of code, I honestly think this whole PR could fit in a single file
I think we should avoid duplicating data as much as possible, in particular the names of dependencies that are written elsewhere in the source tree. Ideally this code should be iterating over a JSON file or other file that contains the list of dependencies. The tool names should not be hardcoded anywhere in this PR. I'm working on making that possible here

karkhaz · 2023-03-23T16:03:52Z

scripts/ci/preflight-steps/update-cbmc-viewer.py

@@ -0,0 +1,12 @@
+#!/usr/bin/python3


I'm not sure that it makes sense to have three files for CBMC, Viewer, and Kissat, when the files are almost identical.

I like that you've specified the dependencies in a data structure in dependency_links.py. My suggestion is to have a single script that iterates over that dict, instead of manually specifying the key each time.

for dep_name, info in dependency_links.items(): updater = VersionUpdater(kani_dependencies_path, dependencies_links[dep_name]) updater.run_process()

This way, we can add more dependencies simply by adding to the dict, without adding any new code, because the logic is the same.

(info is unused in the code above but I think it should be passed to run_process or to the VersionUpdater constructor)

karkhaz · 2023-03-23T16:32:10Z

scripts/ci/preflight-steps/update-kani-version.py

+        print(f"Error updating kani version to '{version_number}' in '{file_path}'.")
+
+
+def main():


This file doesn't appear to do anything because it doesn't actually call update_version

karkhaz · 2023-03-23T16:33:14Z

scripts/ci/preflight-steps/update-kani-version.py

+
+def main():
+    if len(sys.argv) != 2:
+        print("Usage: python update_versions.py <new_version>")


Suggest that instead of asking the user to specify the new version, you should read the version from Cargo.toml, reset the patch number and bump the minor number, and use that. Having to specify a version number manually should be optional.

karkhaz · 2023-03-23T16:34:38Z

scripts/ci/preflight-steps/update-kani-version.py

@@ -0,0 +1,45 @@
+#!/usr/bin/python3


The correct shebang line for python (and most other interpreters) is #!/usr/bin/env python3

karkhaz · 2023-03-23T16:36:23Z

scripts/ci/preflight-steps/update-kani-version.py

+            toml.dump(data, f)
+
+        print(f"Version updated succesfully to '{version_number}' in '{file_path}'.")
+    except Exception as e:


I don't see a need to catch Exception here or anywhere else... I suggest removing the try-catch unless there's a specific exception that's likely to be thrown here.

karkhaz · 2023-03-23T16:50:18Z

scripts/ci/preflight-steps/dependency_updater.py

+        else:
+            return re.search(r"v?(\d+\.\d+(\.\d+)?(-\S+)?)", response.url).group(1)
+
+    def read_dependencies(self):


Does this method need to exist? It seems that its reading the current version numbers. But those never get used, they immediately get overwritten by the latest version number in update_dependencies?

karkhaz · 2023-03-23T17:18:32Z

scripts/ci/preflight-steps/dependency_updater.py

+                    f.write(line)
+
+
+    def run_process(self):


It's not clear to me that you need to walk through the file, updating the existing values. It seems much quicker to overwrite the entire file.

If you have a dict like this:

new_versions = { "CBMC_VIEWER": 3.9, "CBMC": 5.99.0, ... }

then I think it would be much clearer to do this:

with open("kani-dependencies", "w") as handle: for tool, version in new_versions.items(): print(f"{tool}={version}", file=handle)

then you don't need the if-statement on line 27, which hard-codes the tool names and will thus need to be updated whenever we add a new dependency.

so my suggestion is:

Get the list of existing tools by opening kani-dependencies, read each line, split on =, return the first part

Turn that list of tools into the new versions dict, using a variation of your get_latest_version method

Write the dict out using the 3 lines of code above, no need to update the existing file, just overwrite it.

I think this would make the code a lot shorter and clearer, and avoid hardcoding the tool names here. kani-dependencies is the only place those names should be written.

karkhaz · 2023-03-23T17:27:01Z

scripts/ci/preflight-steps/dependency_links.py

@@ -0,0 +1,23 @@
+#!/usr/bin/python3


I'm going to propose that we don't duplicate this information, see this

karkhaz · 2023-03-23T17:34:32Z

scripts/ci/preflight-steps/check-outdated.py

+        else:
+            print(f"Command '{command}' executed succesfully, in '{working_directory}'.")
+            print(stdout)
+    except Exception as e:


Not sure that there's an need for try-except here. If Popen raises, I think the program should crash out early.

There's also no need to use shell=True here, please pass the command as a list.

More generally I'm not sure that there's any need to buffer the output and then immediately print it. I think this code could be a lot shorter:

cmds = [ ["cargo", "install", "cargo-outdated"], ["cargo", "outdated", "--workspace"], ] exit_code = 0 for cmd in cmds: proc = subprocess.run(cmd, cwd=working_directory) exit_code += proc.returncode sys.exit(exit_code)

karkhaz · 2023-03-23T17:36:44Z

scripts/ci/preflight-steps/update-cbmc.py

+from dependency_updater import VersionUpdater
+
+# Set the name of the dependency you want to update
+kani_dependencies_path = "/Users/jaisnan/kani/kani-dependencies"


This shouldn't be using the full path, just kani-dependencies should be fine

zhassan-aws · 2023-03-23T17:56:30Z

Another option to consider for updating Rust dependencies is dependabot. It automatically opens a PR when a dependency has a new version. For example, s2n-quic uses it, and yesterday it opened a PR to upgrade Kani to 0.24.

This is an example of how it can be integrated: #1909

The annoying thing at the moment is that it would open one PR for every version update as opposed to batching all of them together. There's an open feature request to add support for grouped updates.

zhassan-aws · 2023-03-23T17:56:58Z

scripts/ci/preflight-steps/check-outdated.py

+
+def main():
+
+    target_directory = "kani"


Should this be working_directory?

jaisnan · 2023-03-23T18:33:33Z

Another option to consider for updating Rust dependencies is dependabot. It automatically opens a PR when a dependency has a new version. For example, s2n-quic uses it, and yesterday it opened a PR to upgrade Kani to 0.24.

This is an example of how it can be integrated: #1909

The annoying thing at the moment is that it would open one PR for every version update as opposed to batching all of them together. There's an open feature request to add support for grouped updates.

I personally think dependabot is a good candidate for this job, it seems like good practice. I could look into how to use it for Kani.

add preflight steps

cd7aa73

jaisnan requested a review from karkhaz March 23, 2023 01:39

Run autopep8 mode

82c4f9e

karkhaz suggested changes Mar 23, 2023

View reviewed changes

zhassan-aws reviewed Mar 23, 2023

View reviewed changes

scripts/ci/preflight-steps/check-outdated.py

def main():

target_directory = "kani"

Copy link

Contributor

zhassan-aws Mar 23, 2023

Choose a reason for hiding this comment

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

Should this be working_directory?

jaisnan self-assigned this Apr 3, 2023

jaisnan closed this Sep 29, 2023

github-actions bot mentioned this pull request Apr 5, 2025

Automatic toolchain upgrade to nightly-2025-04-04 #3991

Merged

		print(f"Error updating kani version to '{version_number}' in '{file_path}'.")


		def main():

Add scripts to automate dependencies #2313

Add scripts to automate dependencies #2313

Uh oh!

Conversation

jaisnan commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes:

Call-outs:

Testing:

Checklist

Uh oh!

karkhaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhassan-aws commented Mar 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaisnan commented Mar 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jaisnan commented Mar 23, 2023 •

edited

Loading