Not use the versions file for NPM tools #24

pete-murphy · 2021-08-10T03:34:32Z

Resolves: #23

I've tried to come up with the minimal change to make this work (if I'm understanding the intended behavior). These changes delay the interpretation of VersionField → Version until the installMethod is called. So resolve has been updated to return a VersionField instead of Version, and that VersionField is passed through getTool and finally to installMethod where, if it's Latest and the tool is an NPM tool, it is converted to the string "latest" (to generate, e.g., npm install purs-tidy@latest), otherwise it's converted to a version string via Version.showVersion. I'm not sure this is the best way of going about this (or if I've even correctly understood the intended logic), but no other ideas have come to my mind 😅 I'm very much open to suggestion.

pete-murphy · 2021-08-10T03:41:12Z

src/Setup/BuildPlan.purs

+resolve :: Json -> Tool -> ExceptT Error Effect (Maybe { tool :: Tool, versionField :: VersionField })
 resolve versionsContents tool = do
  let key = Key.fromTool tool
  field <- getVersionField key


😕 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.

pete-murphy · 2021-08-10T03:43:13Z

src/Setup/Data/VersionField.purs

+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


This module was just for avoiding circular dependencies.

pete-murphy · 2021-08-10T03:46:52Z

src/Setup/Data/Tool.purs

  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


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.

I think we should either:

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

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.

pete-murphy · 2021-08-10T17:57:22Z

src/Setup/Data/Tool.purs

-          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"


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...

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.

thomashoneyman · 2021-08-10T18:04:27Z

src/Setup/BuildPlan.purs

+-- | on the action.yml file. In the case of NPM packages, bypass the action.yml
+-- | file and use 'Latest'.


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.

thomashoneyman · 2021-08-10T18:09:28Z

src/Setup/BuildPlan.purs


 -- | 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)


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

thomashoneyman · 2021-08-10T18:12:35Z

src/Setup/BuildPlan.purs


 -- | 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


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`.

thomashoneyman · 2021-08-10T18:16:29Z

src/Setup/BuildPlan.purs

-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'.
+resolve :: Json -> Tool -> ExceptT Error Effect (Maybe { tool :: Tool, versionField :: VersionField })


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.

thomashoneyman · 2021-08-10T18:18:28Z

src/Setup/GetTool.purs


  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 }


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

thomashoneyman · 2021-08-10T18:22:03Z

@ptrfrncsmrph I think this is a nice start! It looks like you're understanding everything correctly. I think we might have to shuffle things around a bit more so that we read the versions file close to where we determine the install method, just so that there are fewer places where we're plumbing through a VersionField that we're going to manipulate later.

We might even need to change a data type like InstallMethod so that it includes an exact version for tarballs (which we resolve from the versions file) and either Latest or Exact versions for NPM-installed tools (which we don't resolve from the versions file).

pete-murphy · 2021-08-11T00:50:13Z

Thanks for the feedback! I should be able to give it another pass and re-request review in the next couple days

pete-murphy · 2021-08-23T14:48:49Z

Sorry for delay, I've tried starting over with this, but still not satisfied with the result. What I've got so far: main...ptrfrncsmrph:pm/issue-23-attempt-use-string-as-version. There I'm just passing around the version as a String (which could be literal "latest" or the result of Version.showVersion), which gets awkward when needing to check if that string is greater than or equal to "0.18.0", e.g.

We might even need to change a data type like InstallMethod so that it includes an exact version for tarballs (which we resolve from the versions file) and either Latest or Exact versions for NPM-installed tools (which we don't resolve from the versions file).

Trying to think through how this would work: would the installMethod function be merged into resolve so we have something like

resolve :: Json -> Tool -> ExceptT Error Effect (Maybe InstallMethod)

I've started doing something like that here, but resolve ends up being pretty massive and complex and I might be getting off on the wrong foot: main...ptrfrncsmrph:pm/issue-23-attempt-refactor-install-method.

pete-murphy force-pushed the pm/not-use-the-versions-file-for-npm branch from e25c92c to a5436b3 Compare August 10, 2021 03:36

pete-murphy commented Aug 10, 2021

View reviewed changes

pete-murphy force-pushed the pm/not-use-the-versions-file-for-npm branch from a5436b3 to 13fd320 Compare August 10, 2021 03:42

pete-murphy commented Aug 10, 2021

View reviewed changes

Not use the versions file for NPM tools

2a448cc

pete-murphy force-pushed the pm/not-use-the-versions-file-for-npm branch from 13fd320 to 2a448cc Compare August 10, 2021 03:44

pete-murphy commented Aug 10, 2021

View reviewed changes

pete-murphy marked this pull request as ready for review August 10, 2021 17:49

pete-murphy mentioned this pull request Aug 10, 2021

Don't use the versions file for tools installed via NPM #23

Open

pete-murphy commented Aug 10, 2021

View reviewed changes

thomashoneyman reviewed Aug 10, 2021

View reviewed changes

pete-murphy marked this pull request as draft September 5, 2021 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Not use the versions file for NPM tools #24

Not use the versions file for NPM tools #24

pete-murphy commented Aug 10, 2021 •

edited

Loading

pete-murphy Aug 10, 2021 •

edited

Loading

pete-murphy Aug 10, 2021

pete-murphy Aug 10, 2021

thomashoneyman Aug 10, 2021

pete-murphy Aug 10, 2021

thomashoneyman Aug 10, 2021

thomashoneyman Aug 10, 2021

thomashoneyman Aug 10, 2021

thomashoneyman Aug 10, 2021

thomashoneyman Aug 10, 2021

thomashoneyman Aug 10, 2021

thomashoneyman commented Aug 10, 2021

pete-murphy commented Aug 11, 2021

pete-murphy commented Aug 23, 2021

		-- \| on the action.yml file. In the case of NPM packages, bypass the action.yml
		-- \| file and use 'Latest'.

--- | 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`.

Not use the versions file for NPM tools #24

Are you sure you want to change the base?

Not use the versions file for NPM tools #24

Conversation

pete-murphy commented Aug 10, 2021 • edited Loading

pete-murphy Aug 10, 2021 • edited Loading

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

thomashoneyman commented Aug 10, 2021

pete-murphy commented Aug 11, 2021

pete-murphy commented Aug 23, 2021

pete-murphy commented Aug 10, 2021 •

edited

Loading

pete-murphy Aug 10, 2021 •

edited

Loading