Skip to content

Pass on files and excludeFiles from config to registry #967

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Pass on files and excludeFiles from config to registry #967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
315137e
Pass on files and excludedFiles from config to registry
i-am-the-slime Jun 25, 2023
29dfda4
Document `files` and `excludeFiles` in README
i-am-the-slime Jun 25, 2023
7931b8e
Add simple test for parsing config
i-am-the-slime Jun 25, 2023
6efc9fe
Merge branch 'master' into support-exclude-files
f-f Jul 13, 2023
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
6 changes: 6 additions & 0 deletions spaghetto/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,12 @@ package:
location:
githubOwner: owner
githubRepo: repo
# optional, globs of additional files to publish
files:
- "src-extra/**/*.purs"
# optional, globs of files to exclude from publishing
excludeFiles:
- "src/**/*Test.purs"
Comment on lines +292 to +297
Copy link
Member

Choose a reason for hiding this comment

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

This PR might not be aiming at implementing the full colocation feature, but I'd like to see it through to get a feel at how complex it is.

Once we forgo the idea that all sources are in src and all the test files are in test, we need more config than this:

  1. files will add source files to the ones in src
  2. excludeFiles will remove source files from the ones in src
  3. then we'll need a config key to add test files to the ones in test
  4. ..and something to remove test files from the ones in test I guess?

We'd want (3) and (4) for symmetry if we have (1) and (2), but I question the utility of (1) and (4), as they are not necessary for colocation and I can't come up with non-contrived usecases for having them.

Then something else that we'd need to care about (and that it not yet implemented here) is how we deal with the globs for src and test in the presence of colocation.
This is a good starting point, but there might be more occurrences across the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that we could rename the keys to includes and excludes, and do two passes. First, we add anything from includes to the src we take by default. Then we apply excludes to that set (disallowing you from excluding anything you can't exclude, like your spago.yaml). Since we don't publish the test directory anyway I'm not sure if it's necessary that it has special handling.

Copy link
Member

@f-f f-f Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Colocation is not only about publishing though, there's building and testing involved in there too. Modifying the config format happens in core, and we'd need to think the whole feature through when doing so, which includes thinking about how to include files for spago test to pick up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your input @f-f .
I indeed had not considered testing in this change.

For my personal needs, I need a way to exclude certain files from the libraries I publish.
In particular, these are unit tests, end-to-end-tests any kinds of tests in between. Then there are Story.purs files for storybook that I'd like to colocate but not ship.

I personally don't have a need for excluding what to build. I would want to always compile all .purs files in my project including unit tests, stories, and end-to-end tests.

I have done this in the past similarly to this:
image

The problem which I noticed later came when running storybook or spec on an application that dependend on this library it would include the specs and stories from the lib. Not nice.

So I also can't find a use case for 4 in this:

but I question the utility of (1) and (4), as they are not necessary for colocation and I can't come up with non-contrived usecases for having them

Once we forgo the idea that all sources are in src and all the test files are in test

Also here, I'm afraid to question everything at this stage (and I'm probably lacking some background), but is there really this dichotomy of sources and tests? Or does test stand for anything that shouldn't ship?

I personally have quick and slow (end-to-end) tests that I invoke differently (via npm currently), and I have these storybook files which (in my head at least) are more like visual tests without any real assertions.

So actually, I'm not sure from the current README how you'd translate a project with various spago.dhall files (spago.test.dhall, spago.e2e.dhall, spago.stories.dhall) to the new spago.yaml format.
Would you do this with a small top-level workspace and then subfolders for each of those?
In that case I could imagine a use-case for the files key:

d2

Okay, I'll stop writing here now, maybe it makes sense to discuss this via voice one day so I don't run too far in the wrong direction, and let me know if all this is just draining you and is too annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@f-f What change are you requesting?

Copy link
Member

@f-f f-f Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For my personal needs, I need a way to exclude certain files from the libraries I publish.
In particular, these are unit tests, end-to-end-tests any kinds of tests in between. Then there are Story.purs files for storybook that I'd like to colocate but not ship.

I understand your specific usecase, but we can't really afford to just implement a feature that follows a usecase verbatim, or we'd quickly end up with unmaintainable feature bloat - the issue with that is that the interplay between features often incurs quadratic maintenance complexity. Because of this it is worth to take a step back and try to make the feature more generic than what we need right now, so that it could cover more usecases.

In this case, one related usecase that came up at work is that one of the Erlang build tools we use treats the test folder in a certain way that doesn't work with FFI, so we'd need to either make the test folder configurable, or add a way to add sources for spago test to pick up. Since Colocation is also very much about "changing what is picked up by spago test" we could cover both cases with this same feature.

Also here, I'm afraid to question everything at this stage (and I'm probably lacking some background), but is there really this dichotomy of sources and tests? Or does test stand for anything that shouldn't ship?

It's not a dichotomy as much as a convention. As of today:

  • all the sources for a package are in src
  • all the test files for a package are in test
  • both of these folders get built when you build a package, but spago run only includes sources, while spago test will include both
  • all of the src folder - and only that - ends up published

The whole premise of this new feature is that this convention needs to go, so things need to be configurable instead.
With colocation our definition of "source files" would change from "src" to "src minus whatever we exclude", and our definition of "test files" would change from "src + test" to "src minus whatever we exclude, plus test, plus whatever additional test sources"

What change are you requesting?

Given the above reasoning, I think we could get away with:

  • configuration changes:
    • add a package.build.exclude key: a list of globs of files to exclude from the src directory, when building and when publishing
    • add a package.test.include key: a list of globs of files to include when running test (note: these are in addition to the above, and the test directory)
    • add a package.publish.include key: a list of globs of files to add when publishing (these don't have to be source files), to pass it through to the registry
  • propagate all of these new globs to the whole building process, starting from here
  • there might be various ancillary changes, e.g. one I can think about right now is:
    • when you add a package to the package set, we try to read its spago.yaml - we should detect the package.build.exclude key and use the new glob instead of blindly building the whole of src

Would you do this with a small top-level workspace and then subfolders for each of those?

Yes, I would unpack the end-to-end test to be in its own package, that can be spago run when you need it (rather than running every time you need to run tests)

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm up for talking this through if that helps to unpack things!

Though we should then report back here, so there's a trace of what was discussed and what the reasoning was, for when we want to refer to these in the future

Copy link
Member

Choose a reason for hiding this comment

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

We’d need a package.publish.exclude, since this issue in particular is more about removing files from src when publishing than it is about including them. So we’d end up with package.publish.include + package.publish.exclude.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, and then we won't need the package.build.exclude


# Optional
workspace:
Expand Down
10 changes: 10 additions & 0 deletions spaghetto/core/src/Config.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,17 @@ type PublishConfig =
{ version :: Version
, license :: License
, location :: Maybe Location
, files :: Maybe (Array String)
, excludeFiles :: Maybe (Array String)
}

publishConfigCodec :: JsonCodec PublishConfig
publishConfigCodec = CAR.object "PublishConfig"
{ version: Version.codec
, license: License.codec
, location: CAR.optional Location.codec
, files: CAR.optional (CA.array CA.string)
, excludeFiles: CAR.optional (CA.array CA.string)
}

type RunConfig =
Expand Down Expand Up @@ -157,6 +161,8 @@ bundleConfigCodec = CAR.object "BundleConfig"

data BundlePlatform = BundleNode | BundleBrowser

derive instance Eq BundlePlatform

instance Show BundlePlatform where
show = case _ of
BundleNode -> "node"
Expand All @@ -175,6 +181,8 @@ bundlePlatformCodec = CA.Sum.enumSum show (parsePlatform)
-- App bundles with a main fn, while Module does not include a main.
data BundleType = BundleApp | BundleModule

derive instance Eq BundleType

instance Show BundleType where
show = case _ of
BundleApp -> "app"
Expand Down Expand Up @@ -361,6 +369,8 @@ instance Show StatVerbosity where
CompactStats -> "CompactStats"
VerboseStats -> "VerboseStats"

derive instance Eq StatVerbosity

statVerbosityCodec :: JsonCodec StatVerbosity
statVerbosityCodec = CA.Sum.enumSum print parse
where
Expand Down
43 changes: 39 additions & 4 deletions spaghetto/spago.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ workspace:
ref: 68dddd9351f256980454bc2c1d0aea20e4d53fa9
subdir: foreign
registry-lib:
git: https://github.com/purescript/registry-dev.git
ref: 68dddd9351f256980454bc2c1d0aea20e4d53fa9
git: https://github.com/i-am-the-slime/registry-dev.git
ref: 5c81d2f75bd3abe404d4e9e9bb85945758572f22
subdir: lib
packages:
aff:
Expand Down Expand Up @@ -679,6 +679,40 @@ packages:
dependencies:
- functions
- maybe
language-cst-parser:
type: registry
version: 0.12.3
integrity: sha256-0pnjIJMtW9u1ne+fKB4QbaCeqqwWIGE3fjEQOIU0+ks=
dependencies:
- arrays
- console
- const
- control
- effect
- either
- enums
- foldable-traversable
- free
- functions
- functors
- identity
- integers
- lazy
- lists
- maybe
- newtype
- node-process
- numbers
- ordered-collections
- partial
- prelude
- st
- strings
- transformers
- tuples
- typelevel-prelude
- unfoldable
- unsafe-coerce
lazy:
type: registry
version: 6.0.0
Expand Down Expand Up @@ -1119,8 +1153,8 @@ packages:
- variant
registry-lib:
type: git
url: https://github.com/purescript/registry-dev.git
rev: 68dddd9351f256980454bc2c1d0aea20e4d53fa9
url: https://github.com/i-am-the-slime/registry-dev.git
rev: 5c81d2f75bd3abe404d4e9e9bb85945758572f22
subdir: lib
dependencies:
- aff
Expand All @@ -1139,6 +1173,7 @@ packages:
- functors
- graphs
- integers
- language-cst-parser
- lists
- maybe
- newtype
Expand Down
4 changes: 2 additions & 2 deletions spaghetto/spago.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ workspace:
registry: 20.0.1
extra_packages:
registry-lib:
git: https://github.com/purescript/registry-dev.git
ref: 68dddd9351f256980454bc2c1d0aea20e4d53fa9
git: https://github.com/i-am-the-slime/registry-dev.git
ref: 5c81d2f75bd3abe404d4e9e9bb85945758572f22
subdir: lib
registry-foreign:
git: https://github.com/purescript/registry-dev.git
Expand Down
13 changes: 7 additions & 6 deletions spaghetto/src/Spago/Command/Publish.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ module Spago.Command.Publish (publish, PublishEnv) where
import Spago.Prelude

import Data.Array as Array
import Data.Array.NonEmpty as NonEmptyArray
import Data.Codec.Argonaut as CA
import Data.List as List
import Data.Map as Map
import Data.Set as Set
import Data.Set.NonEmpty (NonEmptySet)
import Data.String.NonEmpty as NonEmptyString
import Data.Tuple as Tuple
import Effect.Ref as Ref
import Node.Path as Path
Expand Down Expand Up @@ -171,7 +173,8 @@ publish _args = do
, version: publishConfig.version
, license: publishConfig.license
, owners: Nothing -- TODO specify owners in spago config
, files: Nothing -- TODO specify files in spago config
, files: NonEmptyArray.fromArray =<< (Array.mapMaybe NonEmptyString.fromString <$> publishConfig.files)
, excludedFiles: NonEmptyArray.fromArray =<< (Array.mapMaybe NonEmptyString.fromString <$> publishConfig.files)
}

unless (Operation.Validation.locationMatches (Manifest manifest) (Metadata metadata)) $ addError $ toDoc
Expand Down Expand Up @@ -209,11 +212,9 @@ publish _args = do
Git.getCleanRef Nothing >>= case _ of
Left err -> addError err
Right ref -> do
unlessM (Operation.Validation.containsPursFile (Path.concat [ selected.path, "src" ])) $ addError $ toDoc
[ "Your package has no .purs files in the src directory. "
, "All package sources must be in the `src` directory, with any additional "
, "sources indicated by the `files` key in your manifest."
]
case Operation.Validation.validatePursModule (Path.concat [ selected.path, "src" ]) of
Right _ -> pure unit
Left e -> addError $ toDoc e

when (Operation.Validation.isMetadataPackage (Manifest manifest)) do
addError $ toDoc "The `metadata` package cannot be uploaded to the registry because it is a protected package."
Expand Down
4 changes: 3 additions & 1 deletion spaghetto/test/Spago.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ import Test.Spago.Lock as Lock
import Test.Spec as Spec
import Test.Spec.Reporter as Spec.Reporter
import Test.Spec.Runner as Spec.Runner
import Test.Spago.Config (spec) as Config

main :: Effect Unit
main = Aff.launchAff_ $ Spec.Runner.runSpec [ Spec.Reporter.consoleReporter ] do
Spec.describe "Spago"
Spec.describe "Spago" do
Lock.spec
Config.spec
72 changes: 72 additions & 0 deletions spaghetto/test/Spago/Config.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
module Test.Spago.Config where

import Spago.Prelude

import Data.Codec.Argonaut as CA
import Data.Map as Map
import Registry.PackageName as PackageName
import Registry.Version as Version
import Registry.License as License
import Spago.Core.Config (Dependencies(..))
import Spago.Core.Config (Config, configCodec) as Config
import Test.Spec (Spec)
import Test.Spec as Spec
import Test.Spec.Assertions as Assert
import Registry.Location (Location(..))

spec :: Spec Unit
spec = do
Spec.it "Parses config" do
case parseYaml Config.configCodec validSpagoYAML of
Left error ->
Assert.fail $ "Failed to parse: " <> CA.printJsonDecodeError error
Right config | config /= validConfig ->
Assert.fail ("\n" <> printYaml Config.configCodec config <> "\ndoes not equal\n\n" <> printYaml Config.configCodec validConfig)
Right _ ->
pure unit

validConfig :: Config.Config
validConfig =
{ package: Just
{ name
, description: Nothing
, dependencies: Dependencies (Map.fromFoldable (dependency <$> [ "aff", "affjax" ]))
, bundle: Nothing
, run: Nothing
, test: Nothing
, publish: Just
{ license
, version
, excludeFiles: Just [ "src/**/*Test.purs" ]
, files: Just [ "src-extra/**/*.purs" ]
, location: Just $ GitHub { owner: "purescript" , repo: "spago" , subdir: Nothing }
}
}
, workspace: Nothing
}
where
license = unsafeFromRight (License.parse "BSD-3-Clause")
version = unsafeFromRight (Version.parse "0.93.6")
name :: PackageName
name = unsafeFromRight (PackageName.parse "mypackage")
dependency name = Tuple (unsafeFromRight (PackageName.parse name)) Nothing

validSpagoYAML :: String
validSpagoYAML =
"""
package:
name: mypackage
publish:
version: 0.93.6
license: BSD-3-Clause
location:
githubOwner: purescript
githubRepo: spago
files:
- "src-extra/**/*.purs"
excludeFiles:
- "src/**/*Test.purs"
dependencies:
- aff
- affjax
"""