Support exclude files (#613)

* Allow possibility to exclude files

* Add test for exclude functionality

* Take files and excludeFiles into account when converting spago config to manifest

* Rename excludedFiles to excludeFiles

* Don't error out when excluding files outside of package directory

* Ensure mandatory files are always included

* Rename files and excludeFiles to include and exclude

* Use excludeFiles / includeFiles

* Add links to always-ignored and always-included files

* Apply suggestions from code review

Co-authored-by: Fabrizio Ferrai <[email protected]>

---------

Co-authored-by: Thomas Honeyman <[email protected]>
Co-authored-by: Fabrizio Ferrai <[email protected]>

Author: 3 people

README.md

@@ -112,37 +112,6 @@ All the pre-registry packages will be grandfathered in, see [here for details](#
 You can find some examples of the `Manifest` that has been generated for them in
 the [examples](./examples) folder.
 
-Here we embed a copy of the `Manifest` schema for ease of consultation:
-
-```dhall
-{-
-
-The schema for `purs.json` files.
-
-This object holds all the info that the Registry needs to know about it.
-
--}
-
-let Map = (./Prelude.dhall).Map.Type
-
-let Manifest =
-      -- The name of the package
-      { name : Text
-      -- A short description of the package
-      , description : Optional Text
-      -- The SPDX code for the license under which the code is released
-      , license : Text
-      -- The version of this package
-      , version : Text
-      -- The location where package sources can be found
-      , location : ./Location.dhall
-      -- The packages this package depends on
-      , dependencies : Map Text Text
-      }
-
-in Manifest
-```
-
 Note: the [`Location` schema](./v1/Location.dhall) includes support for packages that are
 not published from the root of the repository, by supplying the (optional) `subdir` field.
 This means that a repository could potentially host several packages (commonly called a "monorepo").

SPEC.md

@@ -199,10 +199,15 @@ All packages in the registry contain a `purs.json` manifest file in their root d
 - `location`: a valid [`Location`](#location)
 - `owners` (optional): a non-empty array of [`Owner`](#owner)
 - `description` (optional): a description of your library as a plain text string, not markdown, up to 300 characters
-- `files` (optional): a non-empty array of globs, where globs are used to match files outside the `src` directory you want included in your package tarball
-  - Globs must contain only `*`, `**`, `/`, `.`, `..`, and characters for Linux file paths. It is not possible to negate a glob (ie. the `!` character), and globs cannot represent a path out of the package source directory.
+- `includeFiles` (optional): a non-empty array of globs, where globs are used to match file paths (in addition to the `src` directory and other [always-included files](#always-included-files)) that you want included in your package tarball
+- `excludeFiles` (optional): a non-empty array of globs, where globs are used to match file paths in your package source to exclude them (in addition to the [always-ignored files](#always-ignored-files)) from your package tarball.
 - `dependencies`: dependencies of your package as key-value pairs where the keys are [`PackageName`](#packagename)s and values are [`Range`](#range)s; this is a required field, but if you have no dependencies you can provide an empty object.
-  - All dependencies you provide must exist in the registry, and the dependency ranges must be solvable (ie. it must be possible to produce a single version of each dependency that satisfies the provided version bounds, including any transitive dependencies).
+
+Note:
+
+- Globs you provide at the `includeFiles` and `excludeFiles` keys must contain only `*`, `**`, `/`, `.`, `..`, and characters for Linux file paths. It is not possible to negate a glob (ie. the `!` character), and globs cannot represent a path out of the package source directory.
+- When packaging your project source, the registry will first "include" your `src` directory and always-included files such as your `purs.json` file. Then it will include files which match globs indicated by the `includeFiles` key ([always-ignored files](#always-ignored-files) cannot be included). Finally, it will apply the excluding globs indicated by the `excludeFiles` key to the included files ([always-included files](#always-included-files) cannot be excluded).
+- Dependencies you provide at the `dependencies` key must exist in the registry, and the dependency ranges must be solvable (ie. it must be possible to produce a single version of each dependency that satisfies the provided version bounds, including any transitive dependencies).
 
 For example:
 
@@ -216,7 +221,8 @@ For example:
     "githubOwner": "purescript",
     "githubRepo": "purescript-control"
   },
-  "files": ["test/**/*.purs"],
+  "include": ["test/**/*.purs"],
+  "exclude": ["test/graphs"],
   "dependencies": { "newtype": ">=3.0.0 <4.0.0", "prelude": ">=4.0.0 <5.0.0" }
 }
 ```
@@ -408,12 +414,16 @@ Finally, the registry will perform some processing on the source code, package t
 3. Always-ignored files (listed below) are removed from the source code.
 4. The remaining code is packaged into a tarball. The tarball MUST NOT exceed 2,000kb, and a warning will be issued for packages over 200kb.
 
+###### Always-Included Files
+
 These files are always included in the tarball, if present:
 
 - The full contents of the `src` directory.
 - The `purs.json`, `spago.yaml`, `spago.dhall` and `packages.dhall`, `bower.json`, and `package.json` manifest formats (in the root of the package).
 - Any README or LICENSE files (in the root of the package).
 
+###### Always-Ignored Files
+
 These files are always excluded from the tarball, regardless of what is specified in the `files` key:
 
 - `.psci`, `.psci_modules`, `.spago`, `node_modules`, `bower_components`, `.git`, `CVS`, `.svn`, and `.hg` directories.

app/src/App/API.purs

@@ -28,8 +28,10 @@ import Data.FoldableWithIndex (foldMapWithIndex)
 import Data.Map as Map
 import Data.Newtype (over, unwrap)
 import Data.Number.Format as Number.Format
+import Data.Set.NonEmpty as NonEmptySet
 import Data.String as String
-import Data.String.NonEmpty as NonEmptyString
+import Data.String.NonEmpty (fromString) as NonEmptyString
+import Data.String.NonEmpty.Internal (toString) as NonEmptyString
 import Data.String.Regex as Regex
 import Effect.Aff as Aff
 import Effect.Ref as Ref
@@ -76,7 +78,7 @@ import Registry.Manifest as Manifest
 import Registry.Metadata as Metadata
 import Registry.Operation (AuthenticatedData, AuthenticatedPackageOperation(..), PackageSetUpdateData, PublishData)
 import Registry.Operation as Operation
-import Registry.Operation.Validation (UnpublishError(..))
+import Registry.Operation.Validation (UnpublishError(..), validateNoExcludedObligatoryFiles)
 import Registry.Operation.Validation as Operation.Validation
 import Registry.Owner as Owner
 import Registry.PackageName as PackageName
@@ -590,7 +592,7 @@ publishRegistry { source, payload, metadata: Metadata metadata, manifest: Manife
   -- We copy over all files that are always included (ie. src dir, purs.json file),
   -- and any files the user asked for via the 'files' key, and remove all files
   -- that should never be included (even if the user asked for them).
-  copyPackageSourceFiles manifest.files { source: packageDirectory, destination: packageSourceDir }
+  copyPackageSourceFiles { includeFiles: manifest.includeFiles, excludeFiles: manifest.excludeFiles, source: packageDirectory, destination: packageSourceDir }
   Log.debug "Removing always-ignored files from the packaging directory."
   removeIgnoredTarballFiles packageSourceDir
 
@@ -899,33 +901,51 @@ formatPursuitResolutions { resolutions, dependenciesDir } =
 -- | Copy files from the package source directory to the destination directory
 -- | for the tarball. This will copy all always-included files as well as files
 -- | provided by the user via the `files` key.
+-- | Finally, it removes any files specified in the globs behind the `excludeFiles` key.
 copyPackageSourceFiles
   :: forall r
-   . Maybe (NonEmptyArray NonEmptyString)
-  -> { source :: FilePath, destination :: FilePath }
+   . { includeFiles :: Maybe (NonEmptyArray NonEmptyString)
+     , excludeFiles :: Maybe (NonEmptyArray NonEmptyString)
+     , source :: FilePath
+     , destination :: FilePath
+     }
   -> Run (LOG + EXCEPT String + AFF + EFFECT + r) Unit
-copyPackageSourceFiles files { source, destination } = do
+copyPackageSourceFiles { includeFiles, excludeFiles, source, destination } = do
   Log.debug $ "Copying package source files from " <> source <> " to " <> destination
 
-  userFiles <- case files of
+  userExcludeFiles <- case excludeFiles of
     Nothing -> pure []
     Just nonEmptyGlobs -> do
       let globs = map NonEmptyString.toString $ NonEmptyArray.toArray nonEmptyGlobs
       { succeeded, failed } <- FastGlob.match source globs
+      -- Since we will only subtract the excluded globs we can safely ignore the failed globs
+      Log.warn $ "The following paths matched by globs in the 'exclude' key are outside your package directory: " <> String.joinWith ", " failed
+      case validateNoExcludedObligatoryFiles succeeded of
+        Right _ -> pure succeeded
+        Left removedObligatoryFiles -> do
+          Log.warn $ "The following paths matched by globs in the 'excludeFiles' key will be included in the tarball and cannot be excluded: " <> String.joinWith ", " (NonEmptySet.toUnfoldable removedObligatoryFiles)
+          pure $ succeeded Array.\\ NonEmptySet.toUnfoldable removedObligatoryFiles
+
+  userFiles <- case includeFiles of
+    Nothing -> pure []
+    Just nonEmptyGlobs -> do
+      let globs = map NonEmptyString.toString $ NonEmptyArray.toArray nonEmptyGlobs
+      { succeeded, failed } <- FastGlob.match source globs
+      let succeededAndNotExcluded = succeeded Array.\\ userExcludeFiles
 
       unless (Array.null failed) do
         Except.throw $ String.joinWith " "
-          [ "Some paths matched by globs in the 'files' key are outside your package directory."
+          [ "Some paths matched by globs in the 'includeFiles' key are outside your package directory."
           , "Please ensure globs only match within your package directory, including symlinks."
           ]
 
-      case NonEmptyArray.fromArray (Array.filter (Regex.test Internal.Path.pursFileExtensionRegex) succeeded) of
+      case NonEmptyArray.fromArray (Array.filter (Regex.test Internal.Path.pursFileExtensionRegex) succeededAndNotExcluded) of
         Nothing -> pure unit
         Just matches -> do
           let fullPaths = map (\path -> Path.concat [ source, path ]) matches
           Operation.Validation.validatePursModules fullPaths >>= case _ of
             Left formattedError ->
-              Except.throw $ "Some PureScript modules listed in the 'files' section of your manifest contain malformed or disallowed module names." <> formattedError
+              Except.throw $ "Some PureScript modules listed in the 'includeFiles' section of your manifest contain malformed or disallowed module names." <> formattedError
             Right _ ->
               pure unit
 
@@ -934,7 +954,7 @@ copyPackageSourceFiles files { source, destination } = do
   includedFiles <- FastGlob.match source includedGlobs
   includedInsensitiveFiles <- FastGlob.match' source includedInsensitiveGlobs { caseSensitive: false }
 
-  let filesToCopy = userFiles <> includedFiles.succeeded <> includedInsensitiveFiles.succeeded
+  let filesToCopy = (userFiles <> includedFiles.succeeded <> includedInsensitiveFiles.succeeded) Array.\\ userExcludeFiles
   let makePaths path = { from: Path.concat [ source, path ], to: Path.concat [ destination, path ], preserveTimestamps: true }
 
   case map makePaths filesToCopy of
@@ -987,6 +1007,8 @@ spagoToManifest :: Spago.Config -> Either String Manifest
 spagoToManifest config = do
   package@{ name, description, dependencies: Spago.Dependencies deps } <- note "Did not find a package in the config" config.package
   publishConfig@{ version, license } <- note "Did not find a `publish` section in the package config" package.publish
+  let includeFiles = NonEmptyArray.fromArray =<< (Array.mapMaybe NonEmptyString.fromString <$> publishConfig.include)
+  let excludeFiles = NonEmptyArray.fromArray =<< (Array.mapMaybe NonEmptyString.fromString <$> publishConfig.exclude)
   location <- note "Did not find a `location` field in the publish config" publishConfig.location
   let
     checkRange :: Tuple PackageName (Maybe Range) -> Either PackageName (Tuple PackageName Range)
@@ -1005,5 +1027,6 @@ spagoToManifest config = do
     , description
     , dependencies
     , owners: Nothing -- TODO Spago still needs to add this to its config
-    , files: Nothing -- TODO Spago still needs to add this to its config
+    , includeFiles
+    , excludeFiles
     }

app/src/App/Legacy/Manifest.purs

@@ -59,9 +59,10 @@ type LegacyManifest =
 
 toManifest :: PackageName -> Version -> Location -> LegacyManifest -> Manifest
 toManifest name version location { license, description, dependencies } = do
-  let files = Nothing
+  let includeFiles = Nothing
+  let excludeFiles = Nothing
   let owners = Nothing
-  Manifest { name, version, location, license, description, dependencies, files, owners }
+  Manifest { name, version, location, license, description, dependencies, includeFiles, excludeFiles, owners }
 
 -- | Attempt to retrieve a license, description, and set of dependencies from a
 -- | PureScript repo that does not have a Registry-supported manifest, but does

app/test/App/API.purs

@@ -216,7 +216,7 @@ copySourceFiles = Spec.hoistSpec identity (\_ -> Assert.Run.runBaseEffects) $ Sp
     writeDirectories (goodDirectories <> Constants.ignoredDirectories <> [ "test" ])
     writeFiles (goodFiles <> Constants.ignoredFiles <> [ Path.concat [ "test", "Main.purs" ] ])
 
-    API.copyPackageSourceFiles Nothing { source, destination }
+    API.copyPackageSourceFiles { includeFiles: Nothing, excludeFiles: Nothing, source, destination }
 
     paths <- FastGlob.match destination [ "**/*" ]
 
@@ -232,21 +232,60 @@ copySourceFiles = Spec.hoistSpec identity (\_ -> Assert.Run.runBaseEffects) $ Sp
 
   Spec.it "Copies user-specified files" \{ source, destination, writeDirectories, writeFiles } -> do
     let
-      userFiles = NonEmptyArray.fromArray =<< sequence [ NonEmptyString.fromString "test/**/*.purs" ]
+      includeFiles = NonEmptyArray.fromArray =<< sequence [ NonEmptyString.fromString "test/**/*.purs" ]
       testDir = [ "test" ]
       testFiles = [ Path.concat [ "test", "Main.purs" ], Path.concat [ "test", "Test.purs" ] ]
 
     writeDirectories (goodDirectories <> testDir)
     writeFiles (goodFiles <> testFiles)
 
-    API.copyPackageSourceFiles userFiles { source, destination }
+    API.copyPackageSourceFiles { includeFiles, excludeFiles: Nothing, source, destination }
 
     paths <- FastGlob.match destination [ "**/*" ]
 
     let acceptedPaths = goodDirectories <> goodFiles <> testDir <> testFiles
 
     for_ acceptedPaths \path -> do
       paths.succeeded `Assert.Run.shouldContain` path
+
+  Spec.it "Does not copy user-specified excluded files" \{ source, destination, writeDirectories, writeFiles } -> do
+    let
+      includeFiles = NonEmptyArray.fromArray =<< sequence [ NonEmptyString.fromString "test/**/*.purs" ]
+      excludeFiles = NonEmptyArray.fromArray =<< sequence [ NonEmptyString.fromString "test/**/Test.purs" ]
+      testDir = [ "test" ]
+      testMain = Path.concat [ "test", "Main.purs" ]
+      testTest = Path.concat [ "test", "Test.purs" ]
+      testFiles = [ testMain, testTest ]
+
+    writeDirectories (goodDirectories <> testDir)
+    writeFiles (goodFiles <> testFiles)
+
+    API.copyPackageSourceFiles { includeFiles, excludeFiles, source, destination }
+
+    paths <- FastGlob.match destination [ "**/*" ]
+
+    let acceptedPaths = goodDirectories <> goodFiles <> testDir <> [ testMain ]
+
+    for_ acceptedPaths \path -> do
+      paths.succeeded `Assert.Run.shouldContain` path
+
+  Spec.it "Won't exclude always included files" \{ source, destination, writeDirectories, writeFiles } -> do
+    let
+      includeFiles = NonEmptyArray.fromArray =<< sequence [ NonEmptyString.fromString "test/**/*.purs" ]
+      excludeFiles = Just $ NonEmptyArray.singleton (NonEmptyString.nes (Proxy :: _ "purs.json"))
+
+    writeDirectories (goodDirectories)
+    writeFiles (goodFiles)
+
+    API.copyPackageSourceFiles { includeFiles, excludeFiles, source, destination }
+
+    paths <- FastGlob.match destination [ "**/*" ]
+
+    let acceptedPaths = goodDirectories <> goodFiles
+
+    for_ acceptedPaths \path -> do
+      paths.succeeded `Assert.Run.shouldContain` path
+
   where
   runBefore :: forall r. Run (EFFECT + r) _
   runBefore = do

lib/src/Manifest.purs

@@ -52,7 +52,8 @@ newtype Manifest = Manifest
   , location :: Location
   , owners :: Maybe (NonEmptyArray Owner)
   , description :: Maybe String
-  , files :: Maybe (NonEmptyArray NonEmptyString)
+  , includeFiles :: Maybe (NonEmptyArray NonEmptyString)
+  , excludeFiles :: Maybe (NonEmptyArray NonEmptyString)
   , dependencies :: Map PackageName Range
   }
 
@@ -79,6 +80,7 @@ codec = Profunctor.wrapIso Manifest $ CA.object "Manifest"
   $ CA.recordPropOptional (Proxy :: _ "description") (Internal.Codec.limitedString 300)
   $ CA.recordProp (Proxy :: _ "location") Location.codec
   $ CA.recordPropOptional (Proxy :: _ "owners") (CA.Common.nonEmptyArray Owner.codec)
-  $ CA.recordPropOptional (Proxy :: _ "files") (CA.Common.nonEmptyArray CA.Common.nonEmptyString)
+  $ CA.recordPropOptional (Proxy :: _ "includeFiles") (CA.Common.nonEmptyArray CA.Common.nonEmptyString)
+  $ CA.recordPropOptional (Proxy :: _ "excludeFiles") (CA.Common.nonEmptyArray CA.Common.nonEmptyString)
   $ CA.recordProp (Proxy :: _ "dependencies") (Internal.Codec.packageMap Range.codec)
   $ CA.record

lib/src/Operation/Validation.purs

@@ -11,8 +11,12 @@ import Data.Either (Either(..))
 import Data.List.NonEmpty (NonEmptyList)
 import Data.Map (Map)
 import Data.Map as Map
-import Data.Maybe (Maybe(..))
+import Data.Maybe (Maybe(..), maybe)
 import Data.Newtype (un)
+import Data.Set (Set)
+import Data.Set as Set
+import Data.Set.NonEmpty (NonEmptySet)
+import Data.Set.NonEmpty as NonEmptySet
 import Data.String as String
 import Data.Time.Duration (Hours(..))
 import Data.Traversable (traverse)
@@ -203,3 +207,24 @@ validatePursModule moduleString = case CST.parsePartialModule moduleString of
       Right unit
     else
       Left $ "Module name is " <> name <> " but PureScript libraries cannot publish modules named: " <> String.joinWith ", " forbiddenModules
+
+-- | These files are always included in the tarball, if present
+alwaysIncludedFilesIfPresent :: Set FilePath
+alwaysIncludedFilesIfPresent = Set.fromFoldable
+  [ "purs.json"
+  , "spago.yaml"
+  , "spago.dhall"
+  , "packages.dhall"
+  , "bower.json"
+  , "package.json"
+  ]
+
+-- | Validate that the given directory contains no files that are excluded by
+-- | the package's configuration. In case of error, return the set of files that
+-- | shouldn't be excluded
+validateNoExcludedObligatoryFiles :: Array FilePath -> Either (NonEmptySet FilePath) Unit
+validateNoExcludedObligatoryFiles files = do
+  let fileSet = Set.fromFoldable files
+  let removed = Set.intersection alwaysIncludedFilesIfPresent fileSet
+  let removedNonEmpty = NonEmptySet.fromSet removed
+  maybe (Right unit) Left removedNonEmpty

lib/test/Registry/ManifestIndex.purs

@@ -240,5 +240,6 @@ manifestCodec' = Profunctor.dimap to from $ CA.Record.object "ManifestRep"
         }
     , description: Nothing
     , owners: Nothing
-    , files: Nothing
+    , includeFiles: Nothing
+    , excludeFiles: Nothing
     }

lib/test/Registry/Test/Utils.purs

@@ -106,7 +106,8 @@ unsafeManifest name version dependencies = Manifest
       }
   , description: Nothing
   , owners: Nothing
-  , files: Nothing
+  , includeFiles: Nothing
+  , excludeFiles: Nothing
   }
 
 -- | Format a package version as a string in the form '[email protected]'

lib/test/_fixtures/manifests/prelude-4.1.1.json

@@ -13,8 +13,11 @@
       "public": "abc123"
     }
   ],
-  "files": [
+  "includeFiles": [
     "test/**/*.purs"
   ],
+  "excludeFiles": [
+    "test/**/*Spec.purs"
+  ],
   "dependencies": {}
 }