-
Notifications
You must be signed in to change notification settings - Fork 8
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
Not use the versions file for NPM tools #24
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,10 +8,9 @@ import Data.Argonaut.Decode (decodeJson, printJsonDecodeError, (.:)) | |
import Data.Array as Array | ||
import Data.Bifunctor (lmap) | ||
import Data.Either (Either(..)) | ||
import Data.Foldable (fold) | ||
import Data.Foldable (elem, fold) | ||
import Data.Maybe (Maybe(..)) | ||
import Data.Traversable (traverse) | ||
import Data.Version (Version) | ||
import Data.Version as Version | ||
import Effect (Effect) | ||
import Effect.Aff (error, throwError) | ||
|
@@ -20,21 +19,19 @@ import Effect.Exception (Error) | |
import GitHub.Actions.Core as Core | ||
import Setup.Data.Key (Key) | ||
import Setup.Data.Key as Key | ||
import Setup.Data.Tool (Tool) | ||
import Setup.Data.Tool (Tool(..)) | ||
import Setup.Data.Tool as Tool | ||
import Setup.Data.VersionField (VersionField(..)) | ||
import Text.Parsing.Parser (parseErrorMessage) | ||
import Text.Parsing.Parser as ParseError | ||
|
||
-- | The list of tools that should be downloaded and cached by the action | ||
type BuildPlan = Array { tool :: Tool, version :: Version } | ||
type BuildPlan = Array { tool :: Tool, versionField :: VersionField } | ||
|
||
-- | Construct the list of tools that sholud be downloaded and cached by the action | ||
constructBuildPlan :: Json -> ExceptT Error Effect BuildPlan | ||
constructBuildPlan json = map Array.catMaybes $ traverse (resolve json) Tool.allTools | ||
|
||
-- | The parsed value of an input field that specifies a version | ||
data VersionField = Latest | Exact Version | ||
|
||
-- | Attempt to read the value of an input specifying a tool version | ||
getVersionField :: Key -> ExceptT Error Effect (Maybe VersionField) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we move |
||
getVersionField key = do | ||
|
@@ -52,19 +49,24 @@ getVersionField key = do | |
pure (pure (Exact version)) | ||
|
||
-- | Resolve the exact version to provide for a tool in the environment, based | ||
-- | on the action.yml file. | ||
resolve :: Json -> Tool -> ExceptT Error Effect (Maybe { tool :: Tool, version :: Version }) | ||
-- | on the action.yml file. In the case of NPM packages, bypass the action.yml | ||
-- | file and use 'Latest'. | ||
Comment on lines
+52
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't quite right -- we don't want to bypass the |
||
resolve :: Json -> Tool -> ExceptT Error Effect (Maybe { tool :: Tool, versionField :: VersionField }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this functionality should just be baked into the |
||
resolve versionsContents tool = do | ||
let key = Key.fromTool tool | ||
field <- getVersionField key | ||
Comment on lines
+54
to
57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😕 Don't feel good about this approach where I get a |
||
case field of | ||
Nothing -> pure Nothing | ||
case field, tool `elem` [ Psa, PursTidy ] of | ||
Nothing, _ -> pure Nothing | ||
|
||
Just (Exact v) -> liftEffect do | ||
Just (Exact v), _ -> liftEffect do | ||
Core.info "Found exact version" | ||
pure (pure { tool, version: v }) | ||
pure (pure { tool, versionField: Exact v }) | ||
|
||
Just Latest, true -> liftEffect do | ||
Core.info $ fold [ "Fetching latest tag for ", Tool.name tool ] | ||
pure (pure { tool, versionField: Latest }) | ||
|
||
Just Latest -> liftEffect do | ||
Just Latest, false -> liftEffect do | ||
Core.info $ fold [ "Fetching latest tag for ", Tool.name tool ] | ||
|
||
let | ||
|
@@ -77,4 +79,4 @@ resolve versionsContents tool = do | |
throwError $ error "Unable to complete fetching version." | ||
|
||
Right v -> do | ||
pure (pure { tool, version: v }) | ||
pure (pure { tool, versionField: Exact v }) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,18 +3,19 @@ module Setup.Data.Tool where | |
import Prelude | ||
|
||
import Affjax (URL) | ||
import Data.Bounded.Generic (genericBottom, genericTop) | ||
import Data.Either (fromRight') | ||
import Data.Enum (class Enum, upFromIncluding) | ||
import Data.Enum.Generic (genericPred, genericSucc) | ||
import Data.Foldable (elem, fold) | ||
import Data.Generic.Rep (class Generic) | ||
import Data.Bounded.Generic (genericBottom, genericTop) | ||
import Data.Enum.Generic (genericPred, genericSucc) | ||
import Data.Version (Version, parseVersion) | ||
import Data.Version as Version | ||
import Data.Version (parseVersion) | ||
import Node.Path (FilePath) | ||
import Node.Path as Path | ||
import Partial.Unsafe (unsafeCrashWith) | ||
import Setup.Data.Platform (Platform(..), platform) | ||
import Setup.Data.VersionField (VersionField(..)) | ||
import Setup.Data.VersionField as VersionField | ||
|
||
data Tool | ||
= PureScript | ||
|
@@ -91,8 +92,8 @@ type NPMPackage = String | |
|
||
-- | The installation method for a tool, which includes the source path necessary | ||
-- | to download or install the tool. | ||
installMethod :: Tool -> Version -> InstallMethod | ||
installMethod tool version = do | ||
installMethod :: Tool -> VersionField -> InstallMethod | ||
installMethod tool versionField = do | ||
let | ||
toolName = name tool | ||
toolRepo = repository tool | ||
|
@@ -118,26 +119,31 @@ installMethod tool version = do | |
Spago -> Tarball | ||
{ source: formatGitHub' | ||
-- Spago has changed naming conventions from version to version | ||
if version >= unsafeVersion "0.18.1" then case platform of | ||
Windows -> "Windows" | ||
Mac -> "macOS" | ||
Linux -> "Linux" | ||
else if version == unsafeVersion "0.18.0" then case platform of | ||
Windows -> "windows-latest" | ||
Mac -> "macOS-latest" | ||
Linux -> "linux-latest" | ||
else case platform of | ||
Windows -> "windows" | ||
Mac -> "osx" | ||
Linux -> "linux" | ||
case versionField of | ||
Exact version | ||
| version >= unsafeVersion "0.18.1" -> | ||
case platform of | ||
Windows -> "Windows" | ||
Mac -> "macOS" | ||
Linux -> "Linux" | ||
| version == unsafeVersion "0.18.1" -> | ||
case platform of | ||
Windows -> "windows-latest" | ||
Mac -> "macOS-latest" | ||
Linux -> "linux-latest" | ||
_ -> | ||
case platform of | ||
Windows -> "windows" | ||
Mac -> "osx" | ||
Linux -> "linux" | ||
Comment on lines
-121
to
+138
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, I think this is a sign that we haven't got the data structures quite right -- this feels awkward. Ideally, by the time we know we're installing a tarball we also know that we must have an exact version ready to go. |
||
, getExecutablePath: \p -> Path.concat [ p, executableName ] | ||
} | ||
|
||
Psa -> | ||
NPM ("purescript-psa@" <> Version.showVersion version) | ||
NPM ("purescript-psa@" <> VersionField.showVersionField versionField) | ||
|
||
PursTidy -> | ||
NPM ("purs-tidy@" <> Version.showVersion version) | ||
NPM ("purs-tidy@" <> VersionField.showVersionField versionField) | ||
|
||
Zephyr -> Tarball | ||
{ source: formatGitHub' $ case platform of | ||
|
@@ -153,8 +159,8 @@ installMethod tool version = do | |
-- Example: "v0.13.2", "0.15.2" | ||
formatTag :: String | ||
formatTag = do | ||
let versionStr = Version.showVersion version | ||
if tool `elem` [ PureScript, Zephyr, Psa ] then | ||
let versionStr = VersionField.showVersionField versionField | ||
if tool `elem` [ PureScript, Zephyr ] then | ||
Comment on lines
160
to
+163
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should either:
My preference would be (1) because we really don't need the tag for the |
||
fold [ "v", versionStr ] | ||
else | ||
versionStr | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
module Setup.Data.VersionField (VersionField(..), showVersionField) where | ||
|
||
import Data.Version (Version) | ||
import Data.Version as Version | ||
|
||
-- | The parsed value of an input field that specifies a version | ||
data VersionField = Latest | Exact Version | ||
|
||
showVersionField :: VersionField -> String | ||
showVersionField = case _ of | ||
Latest -> "latest" | ||
Exact v -> Version.showVersion v | ||
Comment on lines
+1
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This module was just for avoiding circular dependencies. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,8 +5,6 @@ import Prelude | |
import Control.Monad.Except.Trans (ExceptT, mapExceptT) | ||
import Data.Foldable (fold) | ||
import Data.Maybe (Maybe(..)) | ||
import Data.Version (Version) | ||
import Data.Version as Version | ||
import Effect.Aff (Aff) | ||
import Effect.Class (liftEffect) | ||
import Effect.Exception (Error) | ||
|
@@ -16,18 +14,20 @@ import GitHub.Actions.ToolCache as ToolCache | |
import Setup.Data.Platform (Platform(..), platform) | ||
import Setup.Data.Tool (InstallMethod(..), Tool) | ||
import Setup.Data.Tool as Tool | ||
import Setup.Data.VersionField (VersionField) | ||
import Setup.Data.VersionField as VersionField | ||
|
||
getTool :: { tool :: Tool, version :: Version } -> ExceptT Error Aff Unit | ||
getTool { tool, version } = do | ||
getTool :: { tool :: Tool, versionField :: VersionField } -> ExceptT Error Aff Unit | ||
getTool { tool, versionField } = do | ||
let | ||
name = Tool.name tool | ||
installMethod = Tool.installMethod tool version | ||
installMethod = Tool.installMethod tool versionField | ||
|
||
liftEffect $ Core.info $ fold [ "Fetching ", name ] | ||
|
||
case installMethod of | ||
Tarball opts -> do | ||
mbPath <- mapExceptT liftEffect $ ToolCache.find { arch: Nothing, toolName: name, versionSpec: Version.showVersion version } | ||
mbPath <- mapExceptT liftEffect $ ToolCache.find { arch: Nothing, toolName: name, versionSpec: VersionField.showVersionField versionField } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit long, so perhaps we could import |
||
case mbPath of | ||
Just path -> liftEffect do | ||
Core.info $ fold [ "Found cached version of ", name ] | ||
|
@@ -36,7 +36,7 @@ getTool { tool, version } = do | |
Nothing -> do | ||
downloadPath <- ToolCache.downloadTool' opts.source | ||
extractedPath <- ToolCache.extractTar' downloadPath | ||
cached <- ToolCache.cacheFile { sourceFile: opts.getExecutablePath extractedPath, tool: name, version: Version.showVersion version, targetFile: name, arch: Nothing } | ||
cached <- ToolCache.cacheFile { sourceFile: opts.getExecutablePath extractedPath, tool: name, version: VersionField.showVersionField versionField, targetFile: name, arch: Nothing } | ||
|
||
liftEffect do | ||
Core.info $ fold [ "Cached path ", cached, ", adding to PATH" ] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should expand this documentation comment, because I had to re-read the implementation!