Skip to content

Filter out files that are not ending in .c from c-sources #9200

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
Kleidukos merged 9 commits into from
Aug 24, 2023
Merged

Filter out files that are not ending in .c from c-sources #9200

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a275708
Filter out files that are not ending in .c from c-sources
Kleidukos Aug 23, 2023
b212885
Add names for libraries
Kleidukos Aug 24, 2023
ca584d3
Add name of current component for generic build
Kleidukos Aug 24, 2023
d2831d8
Don't build C sources if there are no valid ".c" files
Kleidukos Aug 24, 2023
eee9fc9
Rework the warnings strings
Kleidukos Aug 24, 2023
af40adb
Add a test
Kleidukos Aug 24, 2023
a73d8c3
bleh
Kleidukos Aug 24, 2023
128e56d
Remove extra whitespace
andreabedini Aug 24, 2023
9e79a1f
Finish off CSourcesSanitisation test
andreabedini Aug 24, 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
2 changes: 1 addition & 1 deletion Cabal-tests/tests/UnitTests/Distribution/Utils/Structured.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ tests = testGroup "Distribution.Utils.Structured"
, testCase "GenericPackageDescription" $
md5Check (Proxy :: Proxy GenericPackageDescription) 0x8d8f340f10a58b8d8a87bf42213dac89
, testCase "LocalBuildInfo" $
md5Check (Proxy :: Proxy LocalBuildInfo) 0xbb22c3258d3092f31e992bc093d09170
md5Check (Proxy :: Proxy LocalBuildInfo) 0x618ab257e99d0b21c617e1f8c39a5a4b
Copy link
Collaborator

@andreabedini andreabedini Aug 24, 2023

Choose a reason for hiding this comment

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

Why is LocalBuildInfo changing?

Copy link
Member Author

@Kleidukos Kleidukos Aug 24, 2023

Choose a reason for hiding this comment

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

No idea

Copy link
Member

@fgaz fgaz Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Try rebasing, it should disappear

Copy link
Collaborator

@andreabedini andreabedini Aug 24, 2023

Choose a reason for hiding this comment

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

🤔

Copy link
Member

@fgaz fgaz Aug 24, 2023

Choose a reason for hiding this comment

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

@andreabedini I'm saying that because this appears to be based on an old commit with a different structured hash (0xbb22... doesn't appear anywhere in HEAD)

Copy link
Member Author

@Kleidukos Kleidukos Aug 24, 2023

Choose a reason for hiding this comment

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

@fgaz rebasing on current 3.10 you mean?

Copy link
Member

@fgaz fgaz Aug 24, 2023

Choose a reason for hiding this comment

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

Nevermind, I grepped the wrong branch 🙈

#endif
]

Expand Down
111 changes: 60 additions & 51 deletions Cabal/src/Distribution/Simple/GHC.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,33 +707,39 @@ buildOrReplLib mReplFlags verbosity numJobs pkg_descr lbi lib clbi = do
-- build any C sources
unless (not has_code || null (cSources libBi)) $ do
info verbosity "Building C Sources..."
sequence_
[ do let baseCcOpts = Internal.componentCcGhcOptions verbosity implInfo
lbi libBi clbi relLibTargetDir filename
vanillaCcOpts = if isGhcDynamic
-- Dynamic GHC requires C sources to be built
-- with -fPIC for REPL to work. See #2207.
then baseCcOpts { ghcOptFPic = toFlag True }
else baseCcOpts
profCcOpts = vanillaCcOpts `mappend` mempty {
ghcOptProfilingMode = toFlag True,
ghcOptObjSuffix = toFlag "p_o"
}
sharedCcOpts = vanillaCcOpts `mappend` mempty {
ghcOptFPic = toFlag True,
ghcOptDynLinkMode = toFlag GhcDynamicOnly,
ghcOptObjSuffix = toFlag "dyn_o"
}
odir = fromFlag (ghcOptObjDir vanillaCcOpts)
createDirectoryIfMissingVerbose verbosity True odir
let runGhcProgIfNeeded ccOpts = do
needsRecomp <- checkNeedsRecompilation filename ccOpts
when needsRecomp $ runGhcProg ccOpts
runGhcProgIfNeeded vanillaCcOpts
unless forRepl $
whenSharedLib forceSharedLib (runGhcProgIfNeeded sharedCcOpts)
unless forRepl $ whenProfLib (runGhcProgIfNeeded profCcOpts)
| filename <- cSources libBi]
let (cSrcs', others) = partition (\filepath -> ".c"`isSuffixOf` filepath) (cSources libBi)
unless (null others) $ do
let files = intercalate ", " others
let libraryName = case libName lib of
Copy link
Collaborator

@andreabedini andreabedini Aug 24, 2023

Choose a reason for hiding this comment

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

We have showLibraryName

Copy link
Member Author

@Kleidukos Kleidukos Aug 24, 2023

Choose a reason for hiding this comment

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

The output is unsatisfying. Especially because it is:

"library '" ++ prettyShow name ++ "'"

So the output would be:

"The following files listed in library 'lib2''s c-sources will not be used:

The double single quotes are not a mistake.

And if it's the main library, it only outputs "library", which breaks the flow of the sentence.

Copy link
Collaborator

@andreabedini andreabedini Aug 24, 2023

Choose a reason for hiding this comment

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

Is

libraryNameStanza :: LibraryName -> String
libraryNameStanza LMainLibName       = "library"
libraryNameStanza (LSubLibName name) = "library " ++ prettyShow name

any better? Otherwise, it's ok.

Copy link
Member

@fgaz fgaz Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Or rephrase as "The following files listed in the c-sources of the library 'lib2' will not be used"

Copy link
Member Author

@Kleidukos Kleidukos Aug 24, 2023

Choose a reason for hiding this comment

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

@fgaz I don't understand what this suggestion is addressing, sorry

Copy link
Member

@fgaz fgaz Aug 24, 2023

Choose a reason for hiding this comment

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

If you phrase it like that, you can use showLibraryName

Copy link
Member Author

@Kleidukos Kleidukos Aug 24, 2023

Choose a reason for hiding this comment

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

But we lose the "main" bit, which is fairly important.

Copy link
Member

@fgaz fgaz Aug 25, 2023

Choose a reason for hiding this comment

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

Good point. I wonder if showLibraryName produces confusing messages elsewhere too. Maybe we should add "main" there too...

Copy link
Member Author

@Kleidukos Kleidukos Aug 25, 2023

Choose a reason for hiding this comment

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

Yes indeed, we will have to formalise a bit more the presentation layer for cabal-install.

LMainLibName -> "the main library"
LSubLibName name -> "library " <> prettyShow name
warn verbosity $ "The following files listed in " <> libraryName <> "'s c-sources will not be used: " <> files
forM_ cSrcs' $ \filename -> do
let baseCcOpts = Internal.componentCcGhcOptions verbosity implInfo
lbi libBi clbi relLibTargetDir filename
vanillaCcOpts = if isGhcDynamic
-- Dynamic GHC requires C sources to be built
-- with -fPIC for REPL to work. See #2207.
then baseCcOpts { ghcOptFPic = toFlag True }
else baseCcOpts
profCcOpts = vanillaCcOpts `mappend` mempty {
ghcOptProfilingMode = toFlag True,
ghcOptObjSuffix = toFlag "p_o"
}
sharedCcOpts = vanillaCcOpts `mappend` mempty {
ghcOptFPic = toFlag True,
ghcOptDynLinkMode = toFlag GhcDynamicOnly,
ghcOptObjSuffix = toFlag "dyn_o"
}
odir = fromFlag (ghcOptObjDir vanillaCcOpts)
createDirectoryIfMissingVerbose verbosity True odir
let runGhcProgIfNeeded ccOpts = do
needsRecomp <- checkNeedsRecompilation filename ccOpts
when needsRecomp $ runGhcProg ccOpts
runGhcProgIfNeeded vanillaCcOpts
unless forRepl $
whenSharedLib forceSharedLib (runGhcProgIfNeeded sharedCcOpts)
unless forRepl $ whenProfLib (runGhcProgIfNeeded profCcOpts)

-- build any JS sources
unless (not has_code || not hasJsSupport || null (jsSources libBi)) $ do
Expand Down Expand Up @@ -1528,31 +1534,34 @@ gbuild verbosity numJobs pkg_descr lbi bm clbi = do

-- build any C sources
unless (null cSrcs) $ do
info verbosity "Building C Sources..."
sequence_
[ do let baseCcOpts = Internal.componentCcGhcOptions verbosity implInfo
info verbosity "Building C Sources..."
let (cSrcs', others) = partition (\filepath -> ".c"`isSuffixOf` filepath) cSrcs
unless (null others) $ do
let files = intercalate ", " others
warn verbosity $ "The following files listed in c-sources will not be used: " <> files
forM_ cSrcs' $ \filename -> do
let baseCcOpts = Internal.componentCcGhcOptions verbosity implInfo
lbi bnfo clbi tmpDir filename
vanillaCcOpts = if isGhcDynamic
-- Dynamic GHC requires C sources to be built
-- with -fPIC for REPL to work. See #2207.
then baseCcOpts { ghcOptFPic = toFlag True }
else baseCcOpts
profCcOpts = vanillaCcOpts `mappend` mempty {
ghcOptProfilingMode = toFlag True
}
sharedCcOpts = vanillaCcOpts `mappend` mempty {
ghcOptFPic = toFlag True,
ghcOptDynLinkMode = toFlag GhcDynamicOnly
}
opts | needProfiling = profCcOpts
| needDynamic = sharedCcOpts
| otherwise = vanillaCcOpts
odir = fromFlag (ghcOptObjDir opts)
createDirectoryIfMissingVerbose verbosity True odir
needsRecomp <- checkNeedsRecompilation filename opts
when needsRecomp $
runGhcProg opts
| filename <- cSrcs ]
let vanillaCcOpts = if isGhcDynamic
-- Dynamic GHC requires C sources to be built
-- with -fPIC for REPL to work. See #2207.
then baseCcOpts { ghcOptFPic = toFlag True }
else baseCcOpts
let profCcOpts = vanillaCcOpts `mappend` mempty {
ghcOptProfilingMode = toFlag True
}
let sharedCcOpts = vanillaCcOpts `mappend` mempty {
ghcOptFPic = toFlag True,
ghcOptDynLinkMode = toFlag GhcDynamicOnly
}
let opts | needProfiling = profCcOpts
| needDynamic = sharedCcOpts
| otherwise = vanillaCcOpts
let odir = fromFlag (ghcOptObjDir opts)
createDirectoryIfMissingVerbose verbosity True odir
needsRecomp <- checkNeedsRecompilation filename opts
when needsRecomp $
runGhcProg opts

-- TODO: problem here is we need the .c files built first, so we can load them
-- with ghci, but .c files can depend on .h files generated by ghc by ffi
Expand Down