Upgrade validation compare deps #19184

dylant-da · 2024-05-13T09:16:04Z

If package2 upgrades package1, all of package2's dependencies should also upgrade their corresponding package1's dependencies

So if mylib-1.0.0 depends on mydep-1.0.0, then mylib-2.0.0 must depend on a mydep >= 1.0.0. If mydep has a version strictly less than 1.0.0, then the dependency downgrades while the package itself upgrades, in which case we should error out.

~~Will be rebased on top of #19123, we should complete a review of that first~~ done

Without explicit references to dependencies' values, the deps won't show up in directDeps. This is fine, but was a real blocker

dylant-da · 2024-05-27T12:13:39Z

I've split this PR into its three pieces (daml, canton, daml) to check that they all work - some additional work is necessary for the first change to pass, we'll remove it in the last step.

dylant-da

Annotated the code in a few spots to help review, as well as ask a few questions

dylant-da · 2024-05-27T12:35:22Z

...cala/com/digitalasset/canton/platform/apiserver/services/admin/PackageUpgradeValidator.scala

      oldPkgId2: Ref.PackageId,
-      optOldPkg2: Option[Ast.Package],
+      optOldPkg2: Option[(Ast.Package, PackageMap)],


We pass in a packagemap of dependencies with each package - this map only contains packageids of deps and their corresponding names and versions, so that we can check that each package's version increases correctly.

dylant-da · 2024-05-27T12:38:34Z

...cala/com/digitalasset/canton/platform/apiserver/services/admin/PackageUpgradeValidator.scala

@@ -199,9 +204,9 @@ class PackageUpgradeValidator(
      case (Some((newPkgId1, newPkg1)), Some((oldPkgId2, oldPkg2))) =>
        strictTypecheckUpgrades(
          typecheckPhase,
-          Some((newPkgId1, newPkg1)),
+          Some((newPkgId1, newPkg1, newPkg1.directDeps.map((x: Ref.PackageId) => (x, packageMap(x))).toMap)),


Here we generate the dependency map for a package from directDeps which is populated during the decoding phase.

An important caveat is that if a dependency does not get used (none of its identifiers show up in the depending package), the dep will show up in the manifest file but will not show up in the directDeps list.

However, I don't think this is much of an issue, it only makes compiler-side checks more restrictive than participant, and the workaround to make it pass on the compiler-side is not to list an unused dependency, which is good code hygiene.

If anything, we should be removing dependencies from the manifest that aren't actually used - should I create a tickt for this?

As discussed on zoom, I think this will let someone sneakily upload v2 of a package (via a dep), without canton checking that v3 upgrades v2 and v2 upgrades v1.

Also as discussed, this is probably not the place to check for that and should be part of another PR.

dylant-da · 2024-05-27T12:38:49Z

sdk/compiler/daml-lf-ast/src/DA/Daml/LF/Ast/Util.hs

+
+-- | Take a package version of regex "(0|[1-9][0-9]*)(\.(0|[1-9][0-9]*))*" into
+-- a list of integers [Integer]
+splitPackageVersion :: PackageVersion -> Maybe [Integer]


utils for versions and comparing them

dylant-da · 2024-05-27T12:39:48Z

sdk/compiler/daml-lf-ast/src/DA/Daml/LF/Ast/Util.hs

+  | SecondVersionUnparseable PackageVersion
+  deriving (Show, Eq, Ord)
+
+comparePackageVersion :: PackageVersion -> PackageVersion -> Either ComparePackageVersionError Ordering


Here we don't use the 0.0.0 convention in other code because (I assume) packages don't adhere to that convention - is that true?

dylant-da · 2024-05-27T12:40:41Z

sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Error.hs

@@ -694,6 +702,10 @@ instance Pretty Warning where
        [ "The template " <> pPrint tpl <> " has implemented interface " <> pPrint iface <> ", which is defined in a previous version of this package."
        , "However, it is recommended that interfaces are defined in their own package separate from their implementations."
        ]
+    WPastDependencyHasUnparseableVersion pkgName version ->
+      "Dependency " <> pPrint pkgName <> " of upgrading package has a version which cannot be parsed: '" <> pPrint version <> "'"
+    WPresentDependencyHasUnparseableVersion pkgName version ->


Backport to 2.x will need to deal with metadata being potentially missing from a dependency.

dylant-da · 2024-05-27T12:43:53Z

sdk/compiler/damlc/tests/src/DA/Test/DamlcUpgrades.hs

@@ -353,18 +358,18 @@ tests damlc =
                      )
                let sharedDir = dir </> "shared"
                let sharedDar = sharedDir </> "out.dar"
-                writeFiles sharedDir (projectFile lfVersion ("upgrades-example-" <> name <> "-dep") Nothing Nothing : sharedDepFiles)


This ugly mess of test code generates daml.yamls and appropriate directories from groups of test files in test-common/.... It's messy but ultimately is just handling the different ways we might want to build a series of packages for upgrading.

dylant-da · 2024-05-27T12:48:31Z

sdk/daml-lf/validation/src/main/scala/com/daml/lf/validation/Upgrading.scala

        tc.check()
    }
  }
+
+  def typecheckUpgrades(


We add this function with the previous interface that didn't take dependencies in order to be able to merge the first part of this PR and build canton against it in step 1 of the merge dance. Part 1: #19274

We remove this function in part 3: #19275

dylant-da · 2024-05-27T12:48:51Z

sdk/daml-lf/validation/src/main/scala/com/daml/lf/validation/Upgrading.scala

      _ <- tryAll(upgradedModules.values, checkModule(_))
    } yield ()
  }

+  private def checkDeps(): Try[Unit] = {


Duplicate the dependency logic from the compiler inside the participant.

dylant-da · 2024-05-27T12:49:30Z

sdk/daml-lf/validation/src/main/scala/com/daml/lf/validation/Upgrading.scala

+  private def checkDep(dep: (Ref.PackageName, Upgrading[Ref.PackageVersion])): Try[Unit] = {
+    val (depName, depVersions) = dep
+    failIf(
+      depVersions.present < depVersions.past,


There already is a "comparable" instance for Ref.PackageVersion that does the correct thing, so we don't need any utils like we did with Haskell

dylant-da · 2024-05-27T12:50:23Z

sdk/test-common/src/main/daml/upgrades/FailsWhenDepsDowngradeVersions/v1/Main.daml

+import Dep
+
+self : Text
+self = "v1 with " <> dep


We have to actually use the dependency, otherwise it won't show up during participant validation in directDeps

paulbrauner-da

Thanks!

...cala/com/digitalasset/canton/platform/apiserver/services/admin/PackageUpgradeValidator.scala

paulbrauner-da · 2024-05-27T13:44:12Z

...cala/com/digitalasset/canton/platform/apiserver/services/admin/PackageUpgradeValidator.scala

@@ -199,9 +204,9 @@ class PackageUpgradeValidator(
      case (Some((newPkgId1, newPkg1)), Some((oldPkgId2, oldPkg2))) =>
        strictTypecheckUpgrades(
          typecheckPhase,
-          Some((newPkgId1, newPkg1)),
+          Some((newPkgId1, newPkg1, newPkg1.directDeps.map((x: Ref.PackageId) => (x, packageMap(x))).toMap)),


As discussed on zoom, I think this will let someone sneakily upload v2 of a package (via a dep), without canton checking that v3 upgrades v2 and v2 upgrades v1.

Also as discussed, this is probably not the place to check for that and should be part of another PR.

paulbrauner-da · 2024-05-27T13:46:18Z

sdk/daml-lf/archive/src/main/scala/com/daml/lf/archive/DecodeV2.scala

    val internedTypes = Work.run(decodeInternedTypes(env0, lfPackage))
    val env = env0.copy(internedTypes = internedTypes)

+    val modules = lfPackage.getModulesList.asScala.map(env.decodeModule(_))


Is that a leftover from a refactoring that didn't happen, or some debugging maybe? It's fine as it is, but I want to make sure you weren't planning on making a change here and then forgot about it.

I decided to move the line here, since it's side-effectful in a way that affects the return value of directDeps in Package.build, to prevent any problems that would occur if the lines were re-ordered.

sdk/daml-lf/validation/src/main/scala/com/daml/lf/validation/Upgrading.scala

paulbrauner-da · 2024-05-27T14:51:57Z

sdk/compiler/daml-lf-ast/src/DA/Daml/LF/Ast/Util.hs

+          let pad xs target
+                | length xs < length target = xs ++ replicate (length target - length xs) 0
+                | otherwise = xs


I think you can make that a bit shorter and (in my opinion) more readable:

let pad xs target = take (length target) (xs ++ repeat 0)

Correct me if I'm wrong, if the target is shorter than xs, then won't this drop elements from xs? We want to keep the maximum of the two.

That said, a hybrid solution would work best:

let pad xs target = take (length target `max` length xs) (xs ++ repeat 0)

Yes, sorry about that!

sdk/compiler/daml-lf-tools/src/DA/Daml/LF/TypeChecker/Upgrade.hs

- Change _package to literal identifier `package` - Use type synonym PackageMap where possible - Fix binding of _new and _deleted in wrong order - Refactor `splitPackageVersion` and `comparePackageVersion`

paulbrauner-da · 2024-05-28T11:58:19Z

sdk/daml-lf/validation/src/main/scala/com/daml/lf/validation/Upgrading.scala

@@ -343,15 +343,15 @@ case class TypecheckUpgrades(
 ) {
  import TypecheckUpgrades._

-  private lazy val _package: Upgrading[Ast.Package] = packages.map(_._2)
+  private lazy val `package`: Upgrading[Ast.Package] = packages.map(_._2)


Oh of course, I understand why the underscore now 🤦

dylant-da force-pushed the upgrade-validation-compare-deps branch from 61ba239 to a167cc3 Compare May 13, 2024 09:32

dylant-da marked this pull request as ready for review May 13, 2024 15:26

dylant-da marked this pull request as draft May 13, 2024 15:26

dylant-da requested review from tudor-da and carlpulley-da May 21, 2024 12:21

dylant-da marked this pull request as ready for review May 21, 2024 13:26

dylant-da added 10 commits May 23, 2024 15:02

Initial implementation

08276e1

Test failure when deps are downgraded

ef9d891

lint

40b4015

return to withTempDir

0906d39

remove compiler_flags from //compiler/damlc/tests:upgrades

ce0c6ee

Validate deps are upgraded correctly on participant side

a93533b

lint

38ff416

Test deps ordering on participant

cea6009

Without explicit references to dependencies' values, the deps won't show up in directDeps. This is fine, but was a real blocker

lint

40f816f

Fix buildifier

5a6d7e0

dylant-da force-pushed the upgrade-validation-compare-deps branch from 0e5a3d8 to 5a6d7e0 Compare May 23, 2024 14:02

This was referenced May 27, 2024

Upgrade validation compare deps pt1 #19274

Merged

Upgrade validation compare deps pt3 #19275

Draft

Add stub function for part 1 compilation

6d58c87

dylant-da commented May 27, 2024

View reviewed changes

paulbrauner-da approved these changes May 27, 2024

View reviewed changes

Apply Paul's suggested changes

bd55261

- Change _package to literal identifier `package` - Use type synonym PackageMap where possible - Fix binding of _new and _deleted in wrong order - Refactor `splitPackageVersion` and `comparePackageVersion`

paulbrauner-da reviewed May 28, 2024

View reviewed changes

paulbrauner-da mentioned this pull request Jun 4, 2024

upgrades check all packages #19328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade validation compare deps #19184

Upgrade validation compare deps #19184

dylant-da commented May 13, 2024 •

edited

dylant-da commented May 27, 2024

dylant-da left a comment

dylant-da May 27, 2024

dylant-da May 27, 2024

paulbrauner-da May 27, 2024

dylant-da May 27, 2024

dylant-da May 27, 2024

dylant-da May 27, 2024

dylant-da May 27, 2024

dylant-da May 27, 2024

dylant-da May 27, 2024

dylant-da May 27, 2024

dylant-da May 27, 2024

paulbrauner-da left a comment

paulbrauner-da May 27, 2024

paulbrauner-da May 27, 2024

dylant-da May 28, 2024 •

edited

paulbrauner-da May 27, 2024

dylant-da May 28, 2024 •

edited

paulbrauner-da May 28, 2024

paulbrauner-da May 28, 2024

Upgrade validation compare deps #19184

Are you sure you want to change the base?

Upgrade validation compare deps #19184

Conversation

dylant-da commented May 13, 2024 • edited

dylant-da commented May 27, 2024

dylant-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulbrauner-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylant-da May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylant-da May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylant-da commented May 13, 2024 •

edited

dylant-da May 28, 2024 •

edited

dylant-da May 28, 2024 •

edited