Skip to content
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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dist/index.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/update.js

Large diffs are not rendered by default.

32 changes: 17 additions & 15 deletions src/Setup/BuildPlan.purs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Copy link
Collaborator

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!

Suggested change
-- | Attempt to read the value of an input specifying a tool version
-- | Attempt to read the value of an input specifying a tool version. If the input is missing
-- | and is not required, then we return `Nothing`. If the input is present and the user has
-- | specified an exact version or the latest version then we return the parsed `VersionField`.

getVersionField :: Key -> ExceptT Error Effect (Maybe VersionField)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move VersionField out to its own module, then I think we ought to move this function to that module as well.

getVersionField key = do
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't quite right -- we don't want to bypass the action.yml because that's where the user specifies "latest" or a specific version. If they give a specific version then we want to use that. It's rather that we want to bypass the versions file when the user wants the 'latest' version of an NPM-installed package.

resolve :: Json -> Tool -> ExceptT Error Effect (Maybe { tool :: Tool, versionField :: VersionField })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this functionality should just be baked into the installMethod function so that there are fewer cases to admit? After all, that's the place where we know explicitly whether a tool is NPM-installed or not.

resolve versionsContents tool = do
let key = Key.fromTool tool
field <- getVersionField key
Comment on lines +54 to 57
Copy link
Author

@pete-murphy pete-murphy Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕 Don't feel good about this approach where I get a VersionField within this function, but then do some transformation of it before returning a conditionally-modified VersionField as output.

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
Expand All @@ -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 })
50 changes: 28 additions & 22 deletions src/Setup/Data/Tool.purs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this Spago case, we know we've got an Exact version for versionField, but that's not represented in the types, so end up having to have a wildcard case. Struggling with how to "make impossible states unrepresentable" here...

Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Expand All @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Psa case here was unreachable—formatTag is only referenced in the PureScript, Spago and Zephyr cases. If it is needed, I would have to expand this a bit so not to prepend v to latest.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should either:

  1. Inline formatTag so that it's obviously only used for the Tarball installation method, because that's the only time we need to bother with the specific format of the tag, or
  2. Keep Psa in here, and add PursTidy, because they do use a v prefix on their tags (unlike, for example, how Spago formats tags with no v prefix).

My preference would be (1) because we really don't need the tag for the NPM install method.

fold [ "v", versionStr ]
else
versionStr
Expand Down
12 changes: 12 additions & 0 deletions src/Setup/Data/VersionField.purs
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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module was just for avoiding circular dependencies.

14 changes: 7 additions & 7 deletions src/Setup/GetTool.purs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit long, so perhaps we could import VersionField as VF?

case mbPath of
Just path -> liftEffect do
Core.info $ fold [ "Found cached version of ", name ]
Expand All @@ -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" ]
Expand Down