Skip to content
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.

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

Added cxx-options and cxx-sources build info fields for separate compilation of C and C++ source files. #4810

Merged
merged 7 commits into from
Oct 5, 2017

Conversation

recursion-ninja
Copy link
Contributor

@recursion-ninja recursion-ninja commented Oct 3, 2017

Added cxx-options and cxx-sources build info fields for separate compilation of C an C++ source files. This resolves issue #3700. If my memory is correct there were some related issues that this should solve in cabal and also in stack. I'll spend a bit of time trying to hunt the issues down and reference them here.

I tested the functionality against the following package:

Here's the requisite checklist:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

I think I probably need to update the documentation somewhere, but I'm not sure where it's located or how to go about that. Would appreciate a bit of help @ezyang.

@recursion-ninja
Copy link
Contributor Author

recursion-ninja commented Oct 3, 2017

It looks like the cabal-testsuites/PackageTests/PreProcessExtraSources/setup.test.hs test is failing.

I'm not sure how to replicate the failure. I did the following:

$ cd cabal-testsuites/PackageTests/PreProcessExtraSources/
$ cabal install --enable-tests --enable-benchmarks #this is the locally built cabal binary
Resolving dependencies...
Configuring PreProcessExtraSources-0.1...
Building PreProcessExtraSources-0.1...
Installed PreProcessExtraSources-0.1

@23Skidoo Is there a different way Travis/appveyor is invoking the test suite?

@23Skidoo
Copy link
Member

23Skidoo commented Oct 3, 2017

Does the procedure described in https://github.com/haskell/cabal/tree/master/cabal-testsuite#how-to-run work for you?

@23Skidoo
Copy link
Member

23Skidoo commented Oct 3, 2017

Also you can try just running cabal configure --enable-tests --enable-benchmarks && cabal build, since that's what the test case in question itself does.

@recursion-ninja
Copy link
Contributor Author

recursion-ninja commented Oct 3, 2017

@23Skidoo, the process described in https://github.com/haskell/cabal/tree/master/cabal-testsuite#how-to-run isn't working for me. I'm having issues with the instruction "Step 1: Build cabal-tests (cabal new-build cabal-tests)." When I run cabal new-build cabal-tests a module I didn't touch fails to compile. However cabal-tests does build when I run stack install.

I tried:

$ cabal-tests cabal-testsuites/PackageTests/PreProcessExtraSources
cabal-tests: Run the 'configure' command first.

I tried:

$ cd cabal-testsuites/PackageTests/PreProcessExtraSources
$ cabal configure --enable-tests --enable-benchmarks
Resolving dependencies...
Configuring PreProcessExtraSources-0.1...
$ cd ../../../
$ cabal-tests cabal-testsuites/PackageTests/PreProcessExtraSources
cabal-tests: Run the 'configure' command first.

I tried:

$ cd cabal-testsuites/PackageTests/PreProcessExtraSources
$ cabal configure --enable-tests --enable-benchmarks && cabal build
Resolving dependencies...
Configuring PreProcessExtraSources-0.1...
Saved package config file is corrupt. Re-run the 'configure' command.

So I'm not sure how to replicate the defect. I can keep changing things I suspect are the issue and pushing changes until Travis & AppVeyor confirm that I fixed the defect.

I'm optimistic that my last commit may have fixed it.

@recursion-ninja
Copy link
Contributor Author

recursion-ninja commented Oct 3, 2017

@23Skidoo, @ezyang I think AppVeyor failed with no fault from my pull request. It says it can't install text. Can I prompt it to rerun somehow?

@23Skidoo
Copy link
Member

23Skidoo commented Oct 3, 2017

Restarted the AppVeyor build.

@recursion-ninja
Copy link
Contributor Author

@23Skidoo, @ezyang, @phadej is there a place in the cabal repository that provides a detailed description of each field that can be specified in a *.cabal file? I'd like to add a detailed description of how the cxx-options and cxx-sources fields behave. Documenting this is (I believe) the last step before the pull request is ready to be merged.

@23Skidoo
Copy link
Member

23Skidoo commented Oct 5, 2017

The c-sources field is documented here, which is generated from this file. I'd put documentation for cxx-sources in the same place.

@recursion-ninja recursion-ninja merged commit b070fc9 into haskell:master Oct 5, 2017
@recursion-ninja
Copy link
Contributor Author

Thanks very much for the guidance with the merge @23Skidoo, and thank you @ezyang for showing me where to start on adding this functionality!

@amigalemming
Copy link
Contributor

I have tried to compile an executable that imports a library using Cxx-Sources.
GHCs ends with:

/usr/bin/ld: haskell/llvm-ffi/dist/build/libHSllvm-ffi-3.8.1.1-8VXDnIFcppwCDsRtxP8bEJ.a(support.o): undefined reference to symbol '__gxx_personality_v0@@CXXABI_1.3'
//usr/lib/x86_64-linux-gnu/libstdc++.so.6: error adding symbols: DSO aus der Kommandozeile fehlt
collect2: error: ld returned 1 exit status

The trick for me so far was to use the g++ linker:

GHC-Options: -pgmlg++

But others may want a different C++ linker.

@amigalemming
Copy link
Contributor

amigalemming commented Nov 1, 2017

When compiling the C++ sources I get a warning about an inappropriate -Wimplicit option for g++:

$ cabal-2.1 build -v
...
Building C++ Sources...
creating dist/build
/opt/ghc/bin/ghc -c -odir dist/build -Idist/build/autogen -Idist/build/global-autogen -Idist/build -Iinclude -I/usr/lib/llvm-3.8/include -optc-O2 '-optc-std=c++0x' '-optc-std=c++11' '-optc-DHS_LLVM_VERSION=308' -hide-all-packages -package-conf dist/package.conf.inplace -package-id enumset-0.0.4-Lj85qfShzo26XBfPyJGMlN -package-id base-4.5.1.0-6e4c9bdc36eeb9121f27ccbbcb62e3f3 cbits/support.cpp
cc1plus: warning: command line option ‘-Wimplicit’ is valid for C/ObjC but not for C++

It seems to be a problem of how GHC calls gcc. At least, Cabal does not mention the -Wimplicit option.

@thoughtpolice
Copy link
Member

The "right way" to link to libstdc++ is probably to instead use

extra-libraries: stdc++

(This is generally better since Cabal expects to list libraries for GHC in certain situations, like using the linker and/or GHCi)

@amigalemming
Copy link
Contributor

Extra-libraries works for me on Linux. I have heard, though, that Extra-libs alone fails on MacOS X. Can someone check?

@angerman
Copy link
Collaborator

angerman commented Nov 1, 2017

I can give https://github.com/recursion-ninja/tcm-memo a test on macOS tomorrow, lest I forget. But that one uses stdc++ in the extra-libs as well.

@angerman
Copy link
Collaborator

angerman commented Nov 1, 2017

If it fails, maybe we can fix it ;-)

@recursion-ninja
Copy link
Contributor Author

recursion-ninja commented Nov 1, 2017

@angerman, I tested the tcm-memo library on my Linux machine and my colleague’s macOS machine. I don't recall which version of macOS my colleague’s machine was running at the time I tested it though. Please give it another test on macOS and let me know the results.

@thoughtpolice, the build info field extra-libraries: stdc++ is probably necessary for most (all?) projects that include C++ source files.

@amigalemming, Hopefully the extra-libraries: stdc++ build info field provides a portable solution to your linking error. Could you give me a bit more context as to your project was using the C++ sources? I don't remember encountering a -Wimplicit warning during my limited testing. It might be symptomatic of passing the wrong default flags to gcc.

@angerman
Copy link
Collaborator

angerman commented Nov 2, 2017

Apart from

src/Data/TCM/Memoized/FFI.hsc:85:10: error:
    Duplicate instance declarations:
      instance Arbitrary CULong
        -- Defined at src/Data/TCM/Memoized/FFI.hsc:85:10
      instance Arbitrary CULong -- Defined in ‘Test.QuickCheck.Arbitrary’
   |
85 | instance Arbitrary CULong where
   |          ^^^^^^^^^^^^^^^^
cabal: Failed to build tcm-memo-1.0.0.0 (which is required by exe:use-the-code
from tcm-memo-1.0.0.0).

this built with new-built just fine on macOS Sierra (10.12.6) with GHC (The Glorious Glasgow Haskell Compilation System, version 8.2.1)

Seems to work as well:

$ use-the-code 1 2        
Cost:
1
Result:
T [3]

@amigalemming
Copy link
Contributor

@angerman The -Wimplicit warning seems to be caused by my rusty GHC-7.4.2 compiler. Starting with GHC-7.6.3 they do not appear anymore. So, you do not have to take any action for Cabal.

@recursion-ninja
Copy link
Contributor Author

recursion-ninja commented Nov 2, 2017

@angerman Thanks for pointing out the fragile build error.

I have added a proper lower version bound to the QuickCheck dependency of the tcm-memo toy project to prevent that in the future.

@amigalemming Glad to hear that the -Wimplicit warning isn't an actionable issue. For sanity's sake, I also added a lower version bound to the base dependency of the tcm-memo project to ensure that it is built with at least GHC-7.6.3.

davetapley added a commit to davetapley/haskell-opencv that referenced this pull request Sep 22, 2018
This is currently broken with:

/usr/include/c++/5/bits/c++0x_warning.h:32:2: error: #error This file
requires compiler and library support for the ISO C++ 2011 standard.
This support must be enabled with the -std=c++11 or -std=gnu++11
compiler options.

I thought this was fixed by haskell/cabal#4810
(which is in Cabal 2.2), but it might be broken because this:
haskell/cabal#5315
(which is in is in Cabal 2.4, which isn't yet in LTS).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants