more flexible da flavour #215
Conversation
|
This should be reviewed in the context of digital-asset/daml#6188. |
garyverhaegen-da
force-pushed
the
more-flexible-da-build
branch
from
9529115 to
fa46eb7
Compare
This PR updates the `CI.hs` script to make the DAML-specific build more flexible. The motivation for this change is to set up a fully auditable CI process for the DAML repo, i.e. one in which we can specify (and track) exactly which commits were involved in creating the version of ghc-lib used by the DAML compiler.
garyverhaegen-da
force-pushed
the
more-flexible-da-build
branch
from
fa46eb7 to
a8f52c6
Compare
There was a problem hiding this comment.
We agree there's a better phrasing though that puts these DA properties on the GhcFlavor type's Da constructor. I appreciate that working out the optparse-applicative magic for this might be a challenge but one way or another we expect it can be achieved and if we can make it happen that would make this so much better 🥰 Please keep me posted!
CI.hs
Outdated
| @@ -48,7 +51,7 @@ data StackOptions = StackOptions | |||
| , ghcOptions :: Maybe String -- If 'Just _', pass '--ghc-options="xxx"' to 'stack build' (for ghc verbose, try 'v3'). | |||
| } deriving (Show) | |||
|
|
|||
| data GhcFlavor = Ghc8101 | Ghc881 | Ghc882 | Ghc883 | DaGhc881 | GhcMaster String | |||
| data GhcFlavor = Ghc8101 | Ghc881 | Ghc882 | Ghc883 | Da | GhcMaster String | |||
There was a problem hiding this comment.
Ideally I'd like to see the DA specific parameters on the Da ctor as we do for the GhcMaster ctor. That String you see there is the commit SHA for GHC in the case of flavor GhcMaster XXX. Better we have one way of doing things so I'd rather see this mechanism extended if possible.
CI.hs
Outdated
| | Ghc881 | ||
| | Ghc882 | ||
| | Ghc883 | ||
| | Da { mergeBaseSha :: String |
There was a problem hiding this comment.
Shouldn't comment on style matters but I think this will fit on a single line eh?
There was a problem hiding this comment.
I'm fine with whatever style you prefer on this project. I've updated this one (though you need longer-than-80-chars lines for it to fit ;-) ); don't hesitate to point out others. In particular, I have no idea how the parseOptions function "should" be indented.
There was a problem hiding this comment.
"Consistency is the hobgoblin of little minds" 🤣
| Ghc8101 -> "--ghc-flavor ghc-8.10.1" | ||
| Ghc881 -> "--ghc-flavor ghc-8.8.1" | ||
| Ghc882 -> "--ghc-flavor ghc-8.8.2" | ||
| Ghc883 -> "--ghc-flavor ghc-8.8.3" | ||
| Da -> "--ghc-flavor " <> daFlavor | ||
| Da { flavor } -> "--ghc-flavor " <> flavor |
There was a problem hiding this comment.
This is good. That is the flag that gets pushed "downstream" into Ghclibgen. I like it.
| "ghc-master" -> Right (GhcMaster current) | ||
| hash -> Right (GhcMaster hash) | ||
| parseDaOptions :: Opts.Parser GhcFlavor | ||
| parseDaOptions = | ||
| Opts.flag' Da ( Opts.long "da" <> Opts.help "Enables DA custom build." ) |
There was a problem hiding this comment.
Maybe da-ghc-flavor? Something to think about. I guess this has to be exclusive with respect to ghc-flavor. Does the parser enforce that?
There was a problem hiding this comment.
That is my understanding of the documentation, and seems to be confirmed in usage:
$ stack runhaskell --package extra --package optparse-applicative CI.hs -- --da --ghc-flavor "ghc-8.8.1"
Invalid option `--ghc-flavor'
Usage: CI.hs (--da [--da-merge-base-sha ARG] [--da-patch ARG]
[--da-gen-flavor ARG] |
--ghc-flavor ARG) [--stack-yaml ARG] [--resolver ARG]
[--verbosity ARG] [--cabal-verbose] [--ghc-options ARG]
Build (and possibly upload) ghc-lib and ghc-lib-parser tarballs.
$ stack runhaskell --package extra --package optparse-applicative CI.hs -- --ghc-flavor "ghc-8.8.1" --da
Invalid option `--da'
Usage: CI.hs (--da [--da-merge-base-sha ARG] [--da-patch ARG]
[--da-gen-flavor ARG] |
--ghc-flavor ARG) [--stack-yaml ARG] [--resolver ARG]
[--verbosity ARG] [--cabal-verbose] [--ghc-options ARG]
Build (and possibly upload) ghc-lib and ghc-lib-parser tarballs.
$
There was a problem hiding this comment.
Which makes me think maybe the other flags don't need the da prefix? 🤔 This may be veering close to bikeshedding territory though. Happy to go with whatever you prefer for all flag names.
There was a problem hiding this comment.
If they don't need the da prefix, let's loose them as an elegance.
There was a problem hiding this comment.
There's no "upload" facilities any more right? Can the help comment be updated accordingly please?
There was a problem hiding this comment.
Dropped da prefix from code and README, and removed the "upload" parenthesis from the help text.
| @@ -197,12 +202,12 @@ buildDists | |||
| Ghc881 -> cmd "cd ghc && git fetch --tags && git checkout ghc-8.8.1-release" | |||
| Ghc882 -> cmd "cd ghc && git fetch --tags && git checkout ghc-8.8.2-release" | |||
| Ghc883 -> cmd "cd ghc && git fetch --tags && git checkout ghc-8.8.3-release" | |||
| Da -> do | |||
| cmd $ "cd ghc && git fetch --tags && git checkout " <> daMergeBaseSha | |||
| Da { mergeBaseSha, patches } -> do | |||
There was a problem hiding this comment.
Exactly what I had in mind! 🎉
This PR updates the
CI.hsscript to make the DAML-specific build more flexible. The motivation for this change is to set up a fully auditable CI process for the DAML repo, i.e. one in which we can specify (and track) exactly which commits were involved in creating the version of ghc-lib used by the DAML compiler.