Skip to content

Configure Renovate to update descriptor dependencies #201

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

Merged
merged 1 commit into from
May 9, 2023

Conversation

gabrielfeo
Copy link
Contributor

@gabrielfeo gabrielfeo commented May 3, 2023

This PR configures Renovate to open PRs updating versions of library descriptor dependencies automatically, aiming to ease the burden on maintainers and contributors :)

The end result can be seen in the fork:

Problem

Dependency descriptors are a good solution and easy to contribute to. However, the common issue of keeping dependencies up-to-date also applies to descriptor dependencies. As a result, when one wants to write a quick-and-dirty notebook today, without the need to pin versions, one gets very old versions of libraries. Pinning versions can be recommended for various reasons, but the default (no pinning) should bring in the latest version.

For example, the notebook below would fail to compile, because while shutdown was introduced in 1.6.0 (and the latest is 1.6.4), the descriptor is still a minor behind at 1.5.2.

%use coroutines
Dispatchers.shutdown()

There are other examples, like slf4j still at 1.7.25 while 2.0.7 is the latest.

Solution

A Renovate config is added. Basically, descriptor properties which refer to a package's version now have a "hint property" below them stating on which package updating should be based on.

{
  "properties": {
    "v": "1.5.2",
    "v-renovate-hint": "update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core"
  },
  "dependencies": [
    "org.jetbrains.kotlinx:kotlinx-coroutines-core:$v",
    "org.jetbrains.kotlinx:kotlinx-coroutines-extras:$v"
  ]
}

The Renovate config has a regex to parse such properties as long as it's immediately followed by a hint property, as well as any dependencies that hardcode versions without properties. More details in the config.

Hint properties are an alternative to comments, which aren't supported in JSON. Migrating to another format isn't feasible because it'd make descriptors incompatible with existing Kernel versions when %useLatestDescriptors is used.

Behavior in old kernel versions

Usages of unnamed properties in notebooks together with %useLatestDescriptors in old versions of the Kernel (before the Kernel was updated to disregard hint properties), will break for multiple-property overrides:

{ "name": "v", "value": "1.0" },
+{ "name": "v-renovate-hint", "value": "..." },
{ "name": "api", "value": "1.0" },
+{ "name": "api-renovate-hint", "value": "..." },
%useLatestDescriptors
// Broken
%use library(2.0, 2.1)

But unnamed overrides of single-property descriptors as below will still work, thanks to migrating all descriptors to the "ordered properties syntax":

-"v": "1.0",
+{ "name": "v", "value": "1.0" },
+{ "name": "v-renovate-hint", "value": "..." },
%useLatestDescriptors
// OK
%use library(2.0)

Enabling Renovate

Renovate can either be activated as a GitHub App or as a GitHub action. It works better as a GitHub app because it responds instantly when a PR checkbox is checked, just like Dependabot, but it'll probably require installation at the Kotlin org-level. If that's not possible, I can add the action to this PR, set to run in a schedule.

@ileasile
Copy link
Collaborator

ileasile commented May 4, 2023

Thank you very much for this PR! It really makes sense to add automation here, but changes in the kernel are of course required first.

And not to break existing notebooks, old JSON files should be left in place for some time. We can possibly stop updating them now, and as people will gradually switch to the new version of kernel, remove them.

The logic for %use library<whatever> should be changed to "search for JSON5 file, and if there is no JSON5, search for JSON in the same place".

As it is quite a big change, I'd like to think twice. Also, isn't it possible to add automation without adding comments? For example, it's the way we do versions completion for our descriptors. doesn't Renovate allow to run any code on demand to extract dependencies?

@gabrielfeo
Copy link
Contributor Author

Hello there!

The logic for %use library<whatever> should be changed to "search for JSON5 file, and if there is no JSON5, search for JSON in the same place".

As it is quite a big change, I'd like to think twice. Also, isn't it possible to add automation without adding comments? For example, it's the way we do versions completion for our descriptors. doesn't Renovate allow to run any code on demand to extract dependencies?

To be honest, I only realized it'd break usages of %useLatestDescriptors after I opened the PR, so I decided to wait and talk to you before choosing another direction. As far as I know, Renovate doesn't support any form of custom code extensions. Here's two alternatives to avoid comments:

1. "Fake" properties instead of the comments (I prefer this one)

  • Pros: clear, concise, still have the "update-base" package right above the version so it's easier to update, Renovate config is still simple (just a minor change to the regex)
  • Cons: kind of a hack, property is exposed to consumers (not a big deal, in my opinion)
{
  "properties": {
-    // update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core
+    "v-metadata": "update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core",
    "v": "1.5.2"
  },
  "dependencies": [
    "org.jetbrains.kotlinx:kotlinx-coroutines-core:$v",
  ]
}

2. Add one matcher in Renovate config per library descriptor

  • Pros: still clear, no regex hacks
  • Cons: verbose, easy to forget about updating the matcher's package when changing a descriptor since they're now separate
  "regexManagers": [
-    // Matches properties commented with "// update: package=group:artifact"
-    {
-      "datasourceTemplate": "maven",
-      "versioningTemplate": "maven",
-      "fileMatch": [".*\\.json5"],
-      "matchStrings": [
-        "\/\/ update: package=(?<packageName>\\S+)\\s+\"(?<depName>.*?)\": ?\"(?<currentValue>.+?)\"",
-        "\/\/ update: package=(?<packageName>\\S+)\\s+{\"name\": \"(?<depName>.*?)\", \"value\": \"(?<currentValue>.+?)\"",
-      ]
-    },
+    // Matches coroutines / 'v' property
+    {
+      "datasourceTemplate": "maven",
+      "versioningTemplate": "maven",
+      "fileMatch": ["coroutines\\.json"],
+      "depNameTemplate": "v",
+      "packageNameTemplate": "org.jetbrains.kotlinx:kotlinx-coroutines-core"
+      "matchStrings": [
+        "\"v\": ?\"(?<currentValue>.+?)\"",
+      ]
+    },
+    // Matches coroutines / 'extrasVersion' property
+    {
+      "datasourceTemplate": "maven",
+      "versioningTemplate": "maven",
+      "fileMatch": ["coroutines\\.json"],
+      "depNameTemplate": "extrasVersion",
+      "packageNameTemplate": "org.jetbrains.kotlinx:kotlinx-coroutines-extras"
+      "matchStrings": [
+        "\"extrasVersion\": ?\"(?<currentValue>.+?)\"",
+      ]
+    },
+    // Matches example / 'version' property
+    {
+      "datasourceTemplate": "maven",
+      "versioningTemplate": "maven",
+      "fileMatch": ["example\\.json"],
+      "depNameTemplate": "version",
+      "packageNameTemplate": "group:artifact"
+      "matchStrings": [
+        "\"version\": ?\"(?<currentValue>.+?)\"",
+      ]
+    },
  ],

@ileasile
Copy link
Collaborator

ileasile commented May 4, 2023

Yes, I like the first approach. It shouldn't cause any issues. I'm only concerned about naming, maybe we should call these properties like <prop-name>-renovate-hint?

@gabrielfeo
Copy link
Contributor Author

Just updated with the first approach. Your name suggestion was better, I used it :)

I verified that Renovate and descriptors (a few, locally) are still working fine.

Shouldn't require any changes to Kernel now, right? I only think there should a note to new contributors about the hints in the "adding libraries" doc. Would you rather update it yourself or can I open a PR?

@ileasile
Copy link
Collaborator

ileasile commented May 4, 2023

I thought a bit, and it seems that this change is also a bit incompatible. Namely, for descriptors that had only one property it was possible to write %use lib(0.1) - without specifying the parameter name. Now they have at least two properties, and it isn't possible. But it's an easy-fixable problem (you need to write %use lib(v=0.1) instead) and only appears when you're using the latest descriptors. So we can go with it. The only thing - I'd like to exclude the properties ending with -renovate-hint on the descriptor deserialization stage in the kernel so it will work as if this property wouldn't exist for the new versions of the kernel.

And one more concern here. We have ordered syntax for properties:

"properties": [
    {"name": "v", "value": "0.1"},
    {"name": "api-v", "value": "1.2"}
]

This allows users to write %use library(0.2, 1.2) instead of using named-parameters-syntax and also workarounds the problem that I've described above. Maybe we can use this syntax instead?

@gabrielfeo
Copy link
Contributor Author

I'd like to exclude the properties ending with -renovate-hint on the descriptor deserialization stage in the kernel so it will work as if this property wouldn't exist for the new versions of the kernel.

Sure, I can work on that.

This allows users to write %use library(0.2, 1.2) instead of using named-parameters-syntax and also workarounds the problem that I've described above. Maybe we can use this syntax instead?

You mean this, just so that "v" comes first, or something else (like alphabetically)?

  "properties": {
-    "v-renovate-hint": "update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core",
-    "v": "1.5.2",
-    "api-renovate-hint": "update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core",
-    "api": "1.5.2"
+    "v": "1.5.2",
+    "v-renovate-hint": "update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core"
+    "api": "1.5.2"
+    "api-renovate-hint": "update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core",
  },

@ileasile
Copy link
Collaborator

ileasile commented May 5, 2023

No, I mean using JSON array instead of JSON object for properties, and also moving the hints to the end of the list :

{
-  "properties": {
-    "v-renovate-hint": "update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core",
-    "v": "1.5.2",
-    "api-renovate-hint": "update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core",
-    "api": "1.5.2"
-  }
+ "properties": [
+    {"name": "v", "value": "1.5.2"},
+    {"name": "api", "value": "1.5.2"},
+    {"name": "v-renovate-hint", "value": "update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core"},
+    {"name": "api-renovate-hint", "value": "update: package=org.jetbrains.kotlinx:kotlinx-coroutines-core"}
+ ],
}

@gabrielfeo
Copy link
Contributor Author

gabrielfeo commented May 5, 2023

Unfortunately, we're limited when it comes to sorting. We need the hint properties to be either immediately above or below its "target" property.

Renovate uses RE2 for Regex, which means I cannot backreference like for every property '<x>', get the version from it, then the packageName from '<x>-renovate-hint' later in the file (in fact the "v" in "v-renovate-hint" has no use in the config, I only prefix hints with their name to avoid duplicate keys). Without backreferences, the solution I have is the current regex: for every property ending in '-renovate-hint', get packageName from it, then get version from the next line. Also, note that the regex must parse the version from the actual property line, not the hint, so that Renovate knows where to replace when updating it.

However, I think we could preserve the current behavior of %use library(0.2, 1.2) by removing hint properties on deserialization as you suggested. We could ensure that when (0.2, 1.2) is parsed to replace the descriptor properties, it is done with the hint properties already removed.

@ileasile
Copy link
Collaborator

ileasile commented May 5, 2023

Ok, let's then unite these approaches:

  1. Use ordered syntax for properties
  2. Place hints right after the corresponding properties
  3. Remove hints properties on the kernel side, I'll do this shortly

@gabrielfeo
Copy link
Contributor Author

So I updated for hints to come after their target properties (number 2). Correct me if I'm wrong, but number 3 (removing hints kernel-side) will already restore number 1 (using ordered syntax).

Also, do we still need to move properties to the full syntax {"name": "v", "value": "1.5.2"},? Why is it necessary?

Add a Renovate config with regex matchers for dependencies
of library descriptors, matching hint properties added to
each descriptor. Hint properties are an alternative to comments,
which aren't supported in JSON. Migrating to another format
isn't feasible because it'd make descriptors incompatible with
existing Kernel versions when `%useLatestDescriptors` is used.

Migrate descriptors to ordered properties syntax to preserve
behavior of single-property unnamed overrides in old Kernel versions.
Multi-property unnamed overrides will break.
@ileasile ileasile merged commit d0b0337 into Kotlin:master May 9, 2023
@ileasile
Copy link
Collaborator

ileasile commented May 9, 2023

@gabrielfeo Thank you, it works cool! I just wanted to ask, can we somehow skip updates to the versions containing "dev" and "SNAPSHOT"? Could you please adjust the config correspondingly?

@gabrielfeo
Copy link
Contributor Author

@ileasile glad to see it working! The dev and SNAPSHOT updates are unexpected, however. Based on the docs, PRs like #218 and #222 shouldn't exist. I'll look into it

Renovate won't update any package versions to unstable versions (e.g. 4.0.0-rc3) unless the current version has the same major.minor.patch and was already unstable (e.g. it was already on 4.0.0-rc2).
https://docs.renovatebot.com/configuration-options/#ignoreunstable

@ileasile
Copy link
Collaborator

ileasile commented May 9, 2023

Maybe versions starting with 0. are also considered unstable? IDK

@gabrielfeo
Copy link
Contributor Author

Yep, I'm thinking the same thing. Let me confirm and we can make a custom rule to match those currently at 0.x, if that's the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants