Always output all packages in build info

[JordanMartinez]

Description of the change

Fixes #1124. I don't think there's any reason not to write out all packages... Does this need a test?

Checklist:

• Added the change to the "Unreleased" section of the changelog
• Added some example of the new feature to the README
• Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊

[f-f]

Ah brilliant, this is a good fix

I would say this doesn't need a test, but as you noted in the issue thread this thing has already regressed twice, which is strongly hinting at needing a test 😄

[JordanMartinez]

Yeah.... I guess it does need a test.

Besides adding such tests, I also added an inline directive for buildInfo. On a separate note, is there a reason why buildInfo is a record containing each value rather than having each field within that record be a top-level value? (e.g. BuildInfo.packages.foo rather than BuildInfo.buildInfo.packages.foo)?

[f-f]

On a separate note, is there a reason why buildInfo is a record containing each value rather than having each field within that record be a top-level value? (e.g. BuildInfo.packages.foo rather than BuildInfo.buildInfo.packages.foo)?

Yes - I don't recall if we have TODOs about this anywhere, but it has that shape so that we can include things like build time, git commit and other non-package-specific info

[JordanMartinez]

On a separate note, is there a reason why buildInfo is a record containing each value rather than having each field within that record be a top-level value? (e.g. BuildInfo.packages.foo rather than BuildInfo.buildInfo.packages.foo)?

Yes - I don't recall if we have TODOs about this anywhere, but it has that shape so that we can include things like build time, git commit and other non-package-specific info

Just to clarify, you are saying there is a reason for the current code below:

module Spago.Generated.BuildInfo where

buildInfo :: { packages :: <recordType>, pursVersion :: String, spagoVersion :: String }
buildInfo =
  { packages: { "foo": "1.2.3", "bar": "1.2.3" }
  , pursVersion: "1.2.3"
  , spagoVersion: "1.2.3"
  }

rather than this alternative implementation:

module Spago.Generated.BuildInfo where

packages :: <recordType>
packages =
  { "foo": "1.2.3"
  , "bar": "1.2.3"
  }

pursVersion :: String
pursVersion = "1.2.3"

spagoVersion :: String
spagoVersion = "1.2.3"

Could you clarify what that reason is?

[f-f]

Oh, I didn't get what you meant then - no specific reason for that, they are pretty much equivalent. If we want to change this we should do it asap, because I won't want to change it liberally once we are out of alpha.

[f-f]

We include in this setup phase only things that are dependent on the paths, because we run the tests in temp directories so we need to fix the paths at this point.

The version of purs is not path-dependent, so we can call it inside the test itself.

Moreover, we have a helper for this already that we can just call from every test, so you don't have to parse the output, etc:

spago/src/Spago/Purs.purs

Line 27 in 478e379

getPurs = Cmd.getExecutable "purs" >>= parseVersionOutput

Suggested change

	pursVersion = do
	let pursBinary = if Process.platform == Just Win32 then "purs.cmd" else "purs"
	Cmd.exec pursBinary [ "--version" ]
	$ Cmd.defaultExecOptions { pipeStdout = false, pipeStderr = false, pipeStdin = StdinNewPipe }

[JordanMartinez]

Done.

[f-f]

Why not just

Suggested change

	import Test.Spago.Build.BuildInfo as BuildBuildInfo
	import Test.Spago.Build.BuildInfo as BuildInfo

[JordanMartinez]

I was following the convention used in BuildPolyRepo 😄

[JordanMartinez]

Oh, I didn't get what you meant then - no specific reason for that, they are pretty much equivalent. If we want to change this we should do it asap, because I won't want to change it liberally once we are out of alpha.

I've opened an issue for this in #1128 along with a few other questions.