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

Kleidukos · 2023-08-23T15:58:21Z

This poc pre-filters file paths given in c-sources to only keep the ones ending in ".c", and outputs an appropriate message.

Should we check if there are ".h" files explicitly and redirect to "includes"?
Should we expand this to cxx-sources?

QA Notes

Clone the repository https://github.com/jtdaugherty/vty-unix
Remove from its cabal file the exported modules, dependencies and executable component
Run cabal build -w ghc-9.2.8
Make sure the output is the following:

Preprocessing library for repro-0.1.0.0..
Building library for repro-0.1.0.0..
Warning: The following files listed in the main library's c-sources will not
be used: cbits/gwinsz.h.
Header files should be in the 'include' or 'install-include' stanza.
See https://cabal.readthedocs.io/en/3.10/cabal-package.html#pkg-field-includes
Preprocessing library 'lib2' for repro-0.1.0.0..
Building library 'lib2' for repro-0.1.0.0..
Warning: The following files listed in library lib2's c-sources will not be
used: cbits/gwinsz.h.
Header files should be in the 'include' or 'install-include' stanza.
See https://cabal.readthedocs.io/en/3.10/cabal-package.html#pkg-field-includes
Preprocessing executable 'exec1' for repro-0.1.0.0..
Building executable 'exec1' for repro-0.1.0.0..
[1 of 1] Compiling Main             ( Main.hs, /home/hecate/Vrac/cabal-9190-repro/dist-newstyle/build/x86_64-linux/ghc-9.4.5/repro-0.1.0.0/x/exec1/build/exec1/exec1-tmp/Main.o )
Warning: The following files listed in exec1's c-sources will not be used:
cbits/gwinsz.h.
Header files should be in the 'include' or 'install-include' stanza.
See https://cabal.readthedocs.io/en/3.10/cabal-package.html#pkg-field-includes
[2 of 2] Linking /home/hecate/Vrac/cabal-9190-repro/dist-newstyle/build/x86_64-linux/ghc-9.4.5/repro-0.1.0.0/x/exec1/build/exec1/exec1

Exit code should be 0 (you can check with echo $?

relates to #9190

@hasufell @mouse07410 @andreabedini @angerman

fixes haskell#9190

andreabedini · 2023-08-24T03:23:24Z

@Kleidukos I thought there was somehing like this somewhere. Let's wait until we figure out what changed the behaviour and how it changed.

Kleidukos · 2023-08-24T05:27:40Z

@andreabedini I'm not opposed to this for master but for 3.10 it's becoming a bit time-sensitive.

andreabedini · 2023-08-24T06:27:01Z

😞 I understand.

Mikolaj

Thank you for adding the warning --- that's progress. Also, thank you for resisting the urge to deduplicate at the last moment before the releases. :)

angerman · 2023-08-24T09:34:53Z

I think the warning should suggest people to use the include or install-include section. And link to the cabal manual where it clearly says headers.

andreabedini · 2023-08-24T10:06:00Z

Cabal/src/Distribution/Simple/GHC.hs

@@ -710,7 +710,10 @@ buildOrReplLib mReplFlags verbosity numJobs pkg_descr lbi lib clbi = do
    let (cSrcs', others) = partition (\filepath -> ".c"`isSuffixOf` filepath) (cSources libBi)
    unless (null others) $ do
      let files = intercalate ", " others
-      warn verbosity $ "The following files listed in c-sources will not be used: " <> files
+      let libraryName = case libName lib of


We have showLibraryName

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.

Is

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

any better? Otherwise, it's ok.

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

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

If you phrase it like that, you can use showLibraryName

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

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

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

andreabedini · 2023-08-24T10:06:54Z

Cabal-tests/tests/UnitTests/Distribution/Utils/Structured.hs

@@ -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


Why is LocalBuildInfo changing?

~~Try rebasing, it should disappear~~

@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)

@fgaz rebasing on current 3.10 you mean?

Nevermind, I grepped the wrong branch 🙈

andreabedini · 2023-08-24T10:10:15Z

@Kleidukos Could you add a test case too? something that has c-sources: file.h file.c. I think this PR will emit a warning (which we can capture), while with the old behaviour nothing was displayed. This would allow us to notice if something changes again.
🙏 🙇 ❤️

andreabedini · 2023-08-24T11:42:56Z

cabal-testsuite/PackageTests/CSourcesSanitisation/cbits/gwinsz.c

I could reproduce with empty files too, if that makes it any easier

Yeah sure let's go with it 👍

Kleidukos · 2023-08-24T11:43:15Z

@andreabedini would you mind running the cabal-test executable and accept the output? It never finishes on my machine.

andreabedini · 2023-08-24T13:25:01Z

@Kleidukos I took some liberties, I hope you are ok with it ☺️

hamishmack · 2023-09-25T01:23:49Z

This breaks the double-conversion package (uses .cc extension in c-sources).

Also the warning did not work because it is only reached if not (null cSrcs'). All the src files in double-conversions have .cc extensions. So everything is ignored and no warning is issued.

angerman · 2023-09-25T01:26:31Z

Well, .cc is c++, so this should be under cxx-sources not c-sources. Treating c++ files as c files is another one of those "often works" scenarios.

haskell#9200 started filtering `.cc` files from `c-sources` and does not always issue a warning (there has to be at least one `.c` file present before it warns you it is ignoring the others). Some packages in hackage (`double-conversion` for instance) rely on the ability to include `C++` source files in `c-sources`. This is not ideal, but we should probably continue to support these packages. This change will relax the filtering, so that only `.h` files are automatically excluded (with a warning). It will also warn if other non `.c` files are present (suggesting the `cxx-sources` since C++ sources are the most likely to be used). The bug that prevented warnings being displayed when no `.c` files were present is fixed.

jasagredo · 2023-11-18T20:00:48Z

Confirmed it works as expected on Mingw with cabal-3.10.2.0. I can confirm also that #9285 works on top of this one as expected:

Warning: The following header files listed in the main library's c-sources
will not be used: cbits/gwinsz.h.
Header files should be in the 'include' or 'install-include' stanza.
See https://cabal.readthedocs.io/en/3.10/cabal-package.html#pkg-field-includes
Warning: The following files listed in the main library's c-sources do not
have the expected '.c' extension cbits/gwinsz.cc.
C++ files should be in the 'cxx-sources' stanza.
See
https://cabal.readthedocs.io/en/3.10/cabal-package.html#pkg-field-cxx-sources

sternenseemann · 2024-01-06T22:20:10Z

Getting a changelog entry on this (retroactively) would maybe be nice, since the warning does not seem to work at all, at least in Setup.hs builds. Just had a fun afternoon debugging this.

Also, I feel like in this sort of case, the chance of people accidentally relying on the »incorrect« behavior is big enough not to have this in a minor point release…

mpscholten · 2024-07-24T04:59:37Z

This change caused one of our dependencies to silently fail. It would have been nice if this was a hard fail.

Fixes NixOS#319365 Build of pdftotext was broken caused by a recent cabal change introduced via haskell/cabal#9200

Kleidukos force-pushed the hecate-9190-ignored-files-in-c-sources branch 2 times, most recently from 7e79a16 to 3a5774b Compare August 23, 2023 17:42

Kleidukos marked this pull request as ready for review August 23, 2023 18:04

Kleidukos added attention: needs-review attention: needs-manual-qa PR is destined for manual QA labels Aug 23, 2023

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

a275708

fixes haskell#9190

Kleidukos force-pushed the hecate-9190-ignored-files-in-c-sources branch from 3a5774b to a275708 Compare August 23, 2023 18:18

Kleidukos mentioned this pull request Aug 24, 2023

cabal-install-3.10 fails with header files in 'c-sources' #9190

Closed

Mikolaj approved these changes Aug 24, 2023

View reviewed changes

Kleidukos added 3 commits August 24, 2023 10:42

Add names for libraries

b212885

Add name of current component for generic build

ca584d3

Don't build C sources if there are no valid ".c" files

d2831d8

andreabedini reviewed Aug 24, 2023

View reviewed changes

Kleidukos added 3 commits August 24, 2023 13:11

Rework the warnings strings

eee9fc9

Add a test

af40adb

bleh

a73d8c3

andreabedini reviewed Aug 24, 2023

View reviewed changes

andreabedini added 2 commits August 24, 2023 20:51

Remove extra whitespace

128e56d

Finish off CSourcesSanitisation test

9e79a1f

Kleidukos merged commit 47eb77b into haskell:3.10 Aug 24, 2023

Kleidukos deleted the hecate-9190-ignored-files-in-c-sources branch August 24, 2023 15:54

Kleidukos mentioned this pull request Sep 25, 2023

c-source must not have c++ source files in it haskell/double-conversion#43

Closed

hamishmack mentioned this pull request Sep 25, 2023

Relax extension .c requirement for c-sources #9285

Merged

jasagredo added the tested-on: windows QA has been successful on Windows label Nov 19, 2023

This was referenced Feb 2, 2024

Rename c-sources to cmm-sources for Cabal 3.10.2.0 (#9200) pepeiborra/Heapsize#1

Open

New version with support for Cabal 3.10.2.0 haskell/double-conversion#52

Closed

mpscholten mentioned this pull request Jul 24, 2024

haskellPackages.pdftotext: break NixOS/nixpkgs#329585

Merged

13 tasks

mpscholten added a commit to mpscholten/nixpkgs that referenced this pull request Jul 24, 2024

haskellPackages.pdftotext: unbreak

99181fe

Fixes NixOS#319365 Build of pdftotext was broken caused by a recent cabal change introduced via haskell/cabal#9200

ulysses4ever mentioned this pull request Aug 15, 2024

ux: add warning when cabal reads from no index cache #10258

Draft

4 tasks

flandweber pushed a commit to flandweber/nixpkgs that referenced this pull request Aug 28, 2024

haskellPackages.pdftotext: unbreak

585c6a8

Fixes NixOS#319365 Build of pdftotext was broken caused by a recent cabal change introduced via haskell/cabal#9200

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Kleidukos commented Aug 23, 2023 •

edited

Loading

andreabedini commented Aug 24, 2023

Kleidukos commented Aug 24, 2023

andreabedini commented Aug 24, 2023

Mikolaj left a comment

angerman commented Aug 24, 2023

andreabedini Aug 24, 2023

Kleidukos Aug 24, 2023

andreabedini Aug 24, 2023

fgaz Aug 24, 2023 •

edited

Loading

Kleidukos Aug 24, 2023

fgaz Aug 24, 2023

Kleidukos Aug 24, 2023

fgaz Aug 25, 2023

Kleidukos Aug 25, 2023

andreabedini Aug 24, 2023

Kleidukos Aug 24, 2023

fgaz Aug 24, 2023 •

edited

Loading

andreabedini Aug 24, 2023

fgaz Aug 24, 2023

Kleidukos Aug 24, 2023

fgaz Aug 24, 2023

andreabedini commented Aug 24, 2023

andreabedini Aug 24, 2023

Kleidukos Aug 24, 2023

Kleidukos commented Aug 24, 2023

andreabedini commented Aug 24, 2023

hamishmack commented Sep 25, 2023

angerman commented Sep 25, 2023

jasagredo commented Nov 18, 2023

sternenseemann commented Jan 6, 2024

mpscholten commented Jul 24, 2024

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

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

Conversation

Kleidukos commented Aug 23, 2023 • edited Loading

QA Notes

andreabedini commented Aug 24, 2023

Kleidukos commented Aug 24, 2023

andreabedini commented Aug 24, 2023

Mikolaj left a comment

Choose a reason for hiding this comment

angerman commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgaz Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgaz Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreabedini commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kleidukos commented Aug 24, 2023

andreabedini commented Aug 24, 2023

hamishmack commented Sep 25, 2023

angerman commented Sep 25, 2023

jasagredo commented Nov 18, 2023

sternenseemann commented Jan 6, 2024

mpscholten commented Jul 24, 2024

Kleidukos commented Aug 23, 2023 •

edited

Loading

fgaz Aug 24, 2023 •

edited

Loading

fgaz Aug 24, 2023 •

edited

Loading