Skip to content

glob expand extra-src-files when tracking for rebuild #7608

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

Merged
merged 2 commits into from
Oct 22, 2021
Merged

glob expand extra-src-files when tracking for rebuild #7608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a337135
glob expand extra-src-files when tracking for rebuild
gbaz Aug 31, 2021
6644c24
changelog
gbaz Sep 13, 2021
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
7 changes: 6 additions & 1 deletion cabal-install/src/Distribution/Client/SourceFiles.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@
-- we cannot "see" easily.
module Distribution.Client.SourceFiles (needElaboratedConfiguredPackage) where

import Control.Monad.IO.Class

import Distribution.Client.ProjectPlanning.Types
import Distribution.Client.RebuildMonad

import Distribution.Solver.Types.OptionalStanza

import Distribution.Simple.Glob (matchDirFileGlobWithDie)
import Distribution.Simple.PreProcess

import Distribution.Types.PackageDescription
Expand All @@ -36,6 +39,7 @@ import Distribution.ModuleName

import Prelude ()
import Distribution.Client.Compat.Prelude
import Distribution.Verbosity (silent)

import System.FilePath

Expand Down Expand Up @@ -146,13 +150,14 @@ needBuildInfo pkg_descr bi modules = do
-- A.hs-boot; need to track both.
findNeededModules ["hs", "lhs", "hsig", "lhsig"]
findNeededModules ["hs-boot", "lhs-boot"]
expandedExtraSrcFiles <- liftIO $ fmap concat . for (extraSrcFiles pkg_descr) $ \fpath -> matchDirFileGlobWithDie silent (\ _ _ -> return []) (specVersion pkg_descr) "." fpath
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we don't want to die with an error message? Is there any risk of false positives that would justify that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in this setting where we're just tracking setting stuff, dying seems weird.

The only other place this gets glob expanded is in the sdist step, where dying makes sense, because you're sdisting something with missing files. But here we're just checking if we need to trigger a reconfigure...

Copy link
Member

@Mikolaj Mikolaj Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Still, if the user thinks the compiler rebuilds his stuff, but in fact it's not, because the glob is incorrect, we should die ASAP, right? Unless false positives are possible. E.g., the user writes too general globs predicting some files/directories that are not all there yet [edit: and so it dies, even though it won't once the files are in]. Is that or any other false positive possible and likely?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only error would be if the glob matches nothing. I'm extremely chary of having any sort of death or error reporting on a purely administrative step. I don't think there's anywhere else inside the Rebuild monad that we die, so it seems like it would be a significant change from how things now occur. A user will notice if a rebuild happens or not. The behavior no rebuild happening is not substantially different than if they put nothing at all in their extra-src-files, or put in something that matches but doesn't match everything they intended, etc.

That it so say, we're not in a good position here to catch if this is correct. The only way to catch if it is correct is to see if the tarball generated by sdist actually builds.

Copy link
Member

Choose a reason for hiding this comment

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

Good point about sdist tarball building. Let it be so.

Regarding possible errors, I thought it can fail in a variety of ways, both due to a syntax error in the glob and due to misssing directories, etc., as in https://github.com/haskell/cabal/blob/master/Cabal/src/Distribution/Simple/Glob.hs#L243-L250

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for the clarification on possible errors, I'm not particularly familiar with the glob code.

Copy link
Member

Choose a reason for hiding this comment

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

On a second thought, I hope syntax errors in globs are caught already when .cabal file is parsed (it definitely is at cabal check, but that would not be enough). So, no point preparing to handle these errors again.

traverse_ needIfExists $ concat
[ cSources bi
, cxxSources bi
, jsSources bi
, cmmSources bi
, asmSources bi
, extraSrcFiles pkg_descr
, expandedExtraSrcFiles
]
for_ (installIncludes bi) $ \f ->
findFileMonitored ("." : includeDirs bi) f
Expand Down
10 changes: 10 additions & 0 deletions changelog.d/pr-7608
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
synopsis: Glob expand extra-source-files when tracking for rebuild
packages: cabal-install
prs: #7608
issues: #4746

description: {

- Since 3.4, extra-source-files have been tracked by cabal-install in the rebuild-checking logic. However, this tracking did not, until this PR, take into account glob-expansion.

}