Implement multi-package composite dars #18000

samuel-williams-da · 2023-12-08T00:15:42Z

Implements composite-dars (Often unofficially called fat-dars)
This is an easy way to bunch together a set of Dars into a single dar, without needing an extra daml.yaml and source files that import every module.

It takes the form of an extension to multi-package.yaml and a flag.

multi-package.yaml

packages:
  - ./my-package1
  - ...
projects:
  - ./my-sub-project
# \/ New! \/
composite-dars:
  - name: main-production # Name of the package containing the listed sub-packages
    version: 0.0.0 # Version of above ^
    packages: # Paths to directories with daml.yamls to include in the composite dar
      - ./test-proj3 
      - ./test-proj4
    dars: # Paths to explicit dars to include in the composite dar
      - ./libs/dars/my-lib.dar
    # Note that both packages and dars are option, there must simply be at least one package OR dar
    # Finally, required output path
    path: ./my-composite-dar.dar
    # Several composite dars can be specified
  - name: another-composite-dar
    ...

To build this dar, we add the --composite-dar=NAME flag to daml build
daml build --composite-dar main-production
This will build the composite dar itself, and any packages it depends on.

We also add --all-composite-dars which does as you'd expect.

Notes:

--all does not build composite dars, but can be used with the composite-dar flag to give the union of their behaviours
composite dars can only be built from the "top" multi-package.yaml, sub-projects cannot have their composite dars built without explicitly setting --multi-package-path to point to them. The CLI will warn you if you attempt to do this, or if you shadow composite-dar names in sub-projects. This behaviour is to avoid awkward shadowing and requiring another flag to specify which composite-dar you want when they collide.
Since the root level packages: and projects: fields in multi-package.yaml are optional, the composite-dar feature can be used without using multi-package build behaviour, by only specifying composite-dars in the multi-package.yaml, and only using the dars list within each dar.
The --composite-dar flag can be specified multiple times to build many composite-dars in one daml build invocation.

Tested locally that codegen still works on the generated dars.

TODO:

Write proper tests for this new behaviour
Check with curtis if this covers the needs of our clients
Consider how this might play with an external build system

samuel-williams-da · 2023-12-11T16:06:30Z

compiler/daml-lf-reader/src/DA/Daml/LF/Reader.hs

+extractNameAndPackageIdFromPath :: FilePath -> (T.Text, LF.PackageId)
+extractNameAndPackageIdFromPath = bimap (T.pack . intercalate "-") (LF.PackageId . T.pack) . fromJust . unsnoc . wordsBy (=='-') . takeBaseName


TODO: Reconsider this safety...
The PackageId can be derived from the ArchivePayload
The unit-id requires the entire package to be read through proto, so we can read the metadata. Not ideal, perhaps theres some way to lazily parse?

I've been looking into this for longer than I wanted but couldn't get anywhere; the main blocker is that proto3-suite/proto3-wire are strict by default because it reads in a Parser monad so later results depend on earlier ones.

One thing we could do is make a stripped down version of the proto files with just ArchivePayload, Package, PackageMetadata and the interned strings, plus the attending haskell files for that, and use that for this purpose, but that sounds like a lot of copying and pasting.

We could also take the daml2/3 break to move the metadata up into an intermediate level, I don't think we've made any promises for the daml3 encoded dars yet?

IThe proto format of LF2 is definitely not set in stone, but I'd double-check with @remyhaemmerle-da whether that's worth it because it would be our first truly breaking change.

samuel-williams-da · 2023-12-11T16:09:54Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/Dar.hs

+
+  let lfVersion =
+        case nubOrd $ LF.versionMajor <$> darLfVersions of
+          -- Note that this will not select dev. We may want to detect if any of the packages are `X.dev` and use this if so (and warn)


Do we want this?

hmm I think here we should just treat X.dev as the upper bound, not as a special case. If any of the included dars are on LF X.dev, that must have been an intentional choice in a nearby daml.yaml, or someone is trying to make a composite dar with an X.dev .dalf that doesn't otherwise interact with the rest, in which case canton will let them know.

From another point of view, other teams will soon start building projects on 2.dev, so this would prevent them from packaging those projects

Agree that .dev should work and be taken as the max - esp. for the dogfooding reasons Moises mentioned.

samuel-williams-da · 2023-12-11T16:10:05Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/Dar.hs

+      conf = mkConfFile name (Just version) [] Nothing [] pkgId
+      -- Dalfs included unique by packageId
+      dalfs = nubOrdOn thd3 $ concatMap dalfsToList darDalfs
+      dar = createArchive name (Just version) unresolvedBuiltinSdkVersion pkgId dalf dalfs "." [] [conf] []


Is using unresolvedBuiltinSdkVersion reasonable here

ooh tough one but I think that's the best we can do, I don't think it would make sense to take e.g. the max of each included dar's sdk because that wouldn't play well with 0.0.0

Agree, but that said - we have custom ordering on our versions such that 0.0.0 is maximum, so that would work. But I do think it's misleading to say a dar was "created" with an sdk version it wasn't. We don't maintain sdk versions of dependencies.

we have custom ordering on our versions such that 0.0.0 is maximum

oh I didn't know/remember this, thanks!

But I do think it's misleading to say a dar was "created" with an sdk version it wasn't

yes exactly

Agree - the unresolved version is sufficient here. We don't need to resolve versions to order them as greatest.

Add path and dars fields to composite-dar

run-all-tests: true

samuel-williams-da · 2024-01-08T11:30:05Z

In a separate PR (after a little discussion), we will add glob support to all .dar fields in the daml.yaml, including composite dars, for ease of use and consistency.

compiler/daml-lf-reader/src/DA/Daml/LF/Reader.hs

akrmn · 2024-01-11T17:44:37Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/Dar.hs

+      pkg = LF.Package { LF.packageLfVersion = lfVersion, LF.packageModules = NM.empty, LF.packageMetadata = Just pkgMeta }
+      (dalf, pkgId) = encodeArchiveAndHash pkg


when I started reviewing this PR I was very curious what would be the main dalf of a composite .dar; it turned out to be a package without any modules. I wonder if anything on daml or canton relies on there being at least one module in a dalf, but I guess we'll find out :)

I'm also a bit anxious about this - atm all composite DAR functionality is only in multi-package.yaml, so we can expect the only people who use it will know it's experimental.

daml-assistant/daml-project-config/DA/Daml/Project/Types.hs

compiler/damlc/daml-package-config/src/DA/Daml/Package/Config.hs

akrmn · 2024-01-12T13:58:53Z

compiler/daml-lf-reader/src/DA/Daml/LF/Reader.hs

+extractNameAndPackageIdFromPath :: FilePath -> (T.Text, LF.PackageId)
+extractNameAndPackageIdFromPath = second LF.PackageId . T.breakOnEnd "-" . T.pack . takeBaseName


I'm sorry I forgot to mention, fst . T.breakOnEnd "-" will keep a trailing hyphen so we need something like this

Suggested change

extractNameAndPackageIdFromPath :: FilePath -> (T.Text, LF.PackageId)

extractNameAndPackageIdFromPath = second LF.PackageId . T.breakOnEnd "-" . T.pack . takeBaseName

extractNameAndPackageIdFromPath :: FilePath -> Maybe (T.Text, LF.PackageId)

extractNameAndPackageIdFromPath = bimapM (T.stripSuffix "-") (pure . LF.PackageId) . T.breakOnEnd "-" . T.pack . takeBaseName

One benefit here is that it forces us to handle the Nothing case, where the basename breaks our assumptions and doesn't actually contain a hyphen

Oh i saw this in the docs and just completely forgot when implementing, cheers.

run-all-tests: true

akrmn · 2024-01-15T14:22:24Z

compiler/damlc/tests/src/DA/Test/DamlcMultiPackage.hs

+        , test "Building a transitive composite dar gives an error including where the dar is" ["--composite-dar=unshadowed-transitive"] "./first" compositeDarShadowingProject
+            $ Left "Found definition for unshadowed-transitive in the following sub-projects"


Why should this fail if there's a single definition of unshadowed-transitive?

akrmn

Thanks for getting this through @samuel-williams-da!
Just one remaining question about the test case Building a transitive composite dar gives an error including where the dar is

dylant-da

Looks great to me too, thank you!

dylant-da · 2024-01-16T12:59:03Z

compiler/damlc/daml-package-config/src/DA/Daml/Package/Config.hs

+    compositeDarObjects <- fromMaybe [] <$> queryMultiPackageConfig ["composite-dars"] multiPackage
+    mpCompositeDars <- traverse parseCompositeDar compositeDarObjects
+    -- n^2 but this list is usually so small that its not worth complicating the logic
+    forM_ mpCompositeDars $ \cd ->


Might be worth spinning this into its own utility function "findDuplicates", since we do this pretty often throughout the codebase I find. We can save it for gardening.

dylant-da · 2024-01-16T13:41:16Z

compiler/damlc/daml-package-config/src/DA/Daml/Package/Config.hs

-      <$> (MultiPackageConfigFields <$> traverse canonicalizePath packagePaths)
+      <$> 
+        ( MultiPackageConfigFields
+            <$> traverse canonicalizePath (mpPackagePaths mpc)


Might be worth getting rid of canonicalizePath here for something without IO, again potential gardening

dylant-da · 2024-01-16T16:12:52Z

compiler/damlc/lib/DA/Cli/Damlc.hs

+            [] -> do
+              hPutStrLn stderr $ "Couldn't find composite dar with the name " <> T.unpack (LF.unPackageName darName) <> " in " <> mpPath multiPackageConfig
+              warnTransitiveCompositeDar stderr multiPackageConfig darName
+              exitFailure


Can we exit with failure after forM is done running, s.t. an error/warning is printed for every composite dar asked for? This maximizes the amount of useful info for a call.

dylant-da · 2024-01-16T16:28:05Z

compiler/damlc/lib/DA/Cli/Damlc.hs

+                  then toNormalizedFilePath' <$> mpPackagePaths multiPackageConfig
+                  else nubOrd $ concatMap (fmap toNormalizedFilePath' . cdPackages) compositeDars
+            void $ uses_ BuildMulti packagePaths
+            liftIO $ traverse_ (buildAndWriteCompositeDar loggerH buildableDataDeps) compositeDars


This call and its behaviour feels like something that could live in Shake, but this PR is big already so best to put into another PR

dylant-da · 2024-01-16T16:36:57Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/Dar.hs

+      pkg = LF.Package { LF.packageLfVersion = lfVersion, LF.packageModules = NM.empty, LF.packageMetadata = Just pkgMeta }
+      (dalf, pkgId) = encodeArchiveAndHash pkg


I'm also a bit anxious about this - atm all composite DAR functionality is only in multi-package.yaml, so we can expect the only people who use it will know it's experimental.

dylant-da · 2024-01-16T16:37:51Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/Dar.hs

+      conf = mkConfFile name (Just version) [] Nothing [] pkgId
+      -- Dalfs included unique by packageId
+      dalfs = nubOrdOn thd3 $ concatMap dalfsToList darDalfs
+      dar = createArchive name (Just version) unresolvedBuiltinSdkVersion pkgId dalf dalfs "." [] [conf] []


Agree - the unresolved version is sufficient here. We don't need to resolve versions to order them as greatest.

dylant-da · 2024-01-16T16:40:00Z

compiler/damlc/daml-compiler/src/DA/Daml/Compiler/Dar.hs

+
+  let lfVersion =
+        case nubOrd $ LF.versionMajor <$> darLfVersions of
+          -- Note that this will not select dev. We may want to detect if any of the packages are `X.dev` and use this if so (and warn)


Agree that .dev should work and be taken as the max - esp. for the dogfooding reasons Moises mentioned.

samuel-williams-da commented Dec 11, 2023

View reviewed changes

samuel-williams-da force-pushed the sw/multi-package-composite-dars branch from f704435 to 5b7c362 Compare December 15, 2023 12:35

samuel-williams-da added 6 commits December 19, 2023 11:25

Implement file and flag parsing

ed9c855

Implement composite dar generation

c7eef87

Add path and dars fields to composite-dar

Cleanup

478bf6a

Added --all-composite-dars flag

cc1ca60

Tweaks from demo

a7293b0

Improve handling of composite-dars with same name

133cefd

samuel-williams-da force-pushed the sw/multi-package-composite-dars branch from 961b4bc to 133cefd Compare December 19, 2023 11:30

samuel-williams-da added 4 commits December 19, 2023 16:33

First batch of tests

d4ea04d

Add composite dar artifact tests

7ec40c5

Add remaining composite-dar tests

6dd69f4

Merge branch 'main' into sw/multi-package-composite-dars

aea06c9

samuel-williams-da marked this pull request as ready for review January 8, 2024 11:26

samuel-williams-da requested review from basvangijzel-DA, remyhaemmerle-da, akrmn, dylant-da, paulbrauner-da and filmackay as code owners January 8, 2024 11:26

Trigger main tests

7834ba6

run-all-tests: true

Merge branch 'main' into sw/multi-package-composite-dars

13224a8

akrmn reviewed Jan 11, 2024

View reviewed changes

compiler/daml-lf-reader/src/DA/Daml/LF/Reader.hs Outdated Show resolved Hide resolved

akrmn reviewed Jan 11, 2024

View reviewed changes

daml-assistant/daml-project-config/DA/Daml/Project/Types.hs Outdated Show resolved Hide resolved

akrmn reviewed Jan 11, 2024

View reviewed changes

compiler/damlc/daml-package-config/src/DA/Daml/Package/Config.hs Show resolved Hide resolved

Address review

a4cfc92

akrmn reviewed Jan 12, 2024

View reviewed changes

Fix extractNameAndPackageIdFromPath

add3bbd

run-all-tests: true

akrmn reviewed Jan 15, 2024

View reviewed changes

akrmn approved these changes Jan 15, 2024

View reviewed changes

dylant-da approved these changes Jan 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement multi-package composite dars #18000

Implement multi-package composite dars #18000

samuel-williams-da commented Dec 8, 2023 •

edited

samuel-williams-da Dec 11, 2023

akrmn Jan 9, 2024

paulbrauner-da Jan 10, 2024

samuel-williams-da Dec 11, 2023

akrmn Jan 11, 2024

dylant-da Jan 16, 2024

samuel-williams-da Dec 11, 2023

akrmn Jan 11, 2024

samuel-williams-da Jan 12, 2024

akrmn Jan 12, 2024

dylant-da Jan 16, 2024

samuel-williams-da commented Jan 8, 2024

akrmn Jan 11, 2024

dylant-da Jan 16, 2024

akrmn Jan 12, 2024

samuel-williams-da Jan 12, 2024

akrmn Jan 15, 2024

akrmn left a comment

dylant-da left a comment

dylant-da Jan 16, 2024

dylant-da Jan 16, 2024

dylant-da Jan 16, 2024

dylant-da Jan 16, 2024

dylant-da Jan 16, 2024

dylant-da Jan 16, 2024

dylant-da Jan 16, 2024

		extractNameAndPackageIdFromPath :: FilePath -> (T.Text, LF.PackageId)
		extractNameAndPackageIdFromPath = bimap (T.pack . intercalate "-") (LF.PackageId . T.pack) . fromJust . unsnoc . wordsBy (=='-') . takeBaseName

		pkg = LF.Package { LF.packageLfVersion = lfVersion, LF.packageModules = NM.empty, LF.packageMetadata = Just pkgMeta }
		(dalf, pkgId) = encodeArchiveAndHash pkg

		extractNameAndPackageIdFromPath :: FilePath -> (T.Text, LF.PackageId)
		extractNameAndPackageIdFromPath = second LF.PackageId . T.breakOnEnd "-" . T.pack . takeBaseName

		, test "Building a transitive composite dar gives an error including where the dar is" ["--composite-dar=unshadowed-transitive"] "./first" compositeDarShadowingProject
		$ Left "Found definition for unshadowed-transitive in the following sub-projects"

Implement multi-package composite dars #18000

Are you sure you want to change the base?

Implement multi-package composite dars #18000

Conversation

samuel-williams-da commented Dec 8, 2023 • edited

TODO:

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

samuel-williams-da commented Jan 8, 2024

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

akrmn left a comment

Choose a reason for hiding this comment

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

samuel-williams-da commented Dec 8, 2023 •

edited