Fix linking issue with `cxx-options` and `cxx-sources` fields in non-library components #5315

recursion-ninja · 2018-05-12T18:34:32Z

Standard 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.

Changes

I have updated the Cabal library's cxx-options and cxx-sources fields to correctly link when building non-library components (benchmark, executable, test-suite). This pull requests should resolve #5309 and complete the feature I (unknowingly) didn't fully implement in #4810 which partially resolved #3700.

Specific fixes

The main culprit of the C++ object not being passed to the linker can be seen here.

I also removed the following warning when invoking hsc2hs while the cxx-options field is present:

cc1: warning: command line option ‘-std=c++11’ is valid for C++/ObjC++ but not for C

Before we merge

I did not add any tests to the test suite. I looked through the Cabal/test and it wasn't immediately apparent where I would add tests for this functionality or what form the tests would take. However I think it would be prudent to add some tests for this functionality before we merge the pull request.

Should the tests be added for Cabal the library or cabal-install the executable?. I assume Cabal, but I might be wrong.
Should the tests be added in an existing module or a new module?
How do I add the new tests into the existing test suite for the Cabal.

I'd appreciate some direction on the testing front @23Skidoo, @amigalemming, @ezyang, @hvr, @quasicomputational.

…ings.

quasicomputational · 2018-05-13T06:19:40Z

RE tests, could one go in cabal-testsuite? Seems like you have a suitable sketch of a test-case in the issue that can more or less be translated directly.

quasicomputational · 2018-05-13T06:20:56Z

Cabal/Distribution/Simple/GHC.hs

+                                  ghcOptFPic        = toFlag True,
+                                  ghcOptDynLinkMode = toFlag GhcDynamicOnly,
+                                  ghcOptObjSuffix   = toFlag "dyn_o"
+                                }


Can this whitespace change noise be removed, please?

Whitespace removed.

quasicomputational · 2018-05-13T06:23:23Z

Cabal/Distribution/PackageDescription/Check.hs

-        ++ "flag."
+           "'"++prefix++": -O[n]' is generally not needed. When building with "
+        ++ " optimisations Cabal automatically adds '-O2' for "++label++" code. "
+        ++ "Setting it yourself interferes with the --disable-optimization flag."


Cabal/tests/ParserTests has some tests for warnings from check, which you might want to add to.

I added the following test files to ensure that the check provides correct warnings:
regressions/cc-options-with-optimization
regressions/cxx-options-with-optimization

quasicomputational · 2018-05-13T06:24:20Z

Cabal/Distribution/Simple/GHC.hs

@@ -1088,15 +1088,15 @@ gbuildSources :: Verbosity
              -> Version -- ^ specVersion
              -> FilePath
              -> GBuildMode
-              -> IO ([FilePath], [FilePath], [ModuleName])
+              -> IO ([FilePath], [FilePath], [FilePath], [ModuleName])


Is it worth defining a data type for these, rather than relying on 4-tuples and remembering which FilePath is which?

I added a labeled "record" in place of the 4-tuple.

quasicomputational · 2018-05-13T06:25:46Z

Cabal/Distribution/Simple/GHC.hs

+          when needsRecomp $
+            runGhcProg opts
+     | filename <- cxxSrcs ]
+


This might be personal taste, but is this nicer than for_ cxxSrcs $ \ filename -> ...?

Oh, never mind. I see it's patterned on the code below. Best to keep them obviously parallel.

quasicomputational · 2018-05-13T06:28:32Z

Cabal/Distribution/Simple/SrcDist.hs

@@ -449,7 +449,7 @@ allSourcesBuildInfo verbosity bi pps modules = do
      in findFileWithExtension fileExts (hsSourceDirs bi) file
    | module_ <- modules ++ otherModules bi ]

-  return $ sources ++ catMaybes bootFiles ++ cSources bi ++ jsSources bi
+  return $ sources ++ catMaybes bootFiles ++ cSources bi ++ cxxSources bi ++ jsSources bi


cabal-testsuite/PackageTests/SDlist has a test for sdist. Might be worth adding to?

I see the test cases in cabal-testsuite directory, but I'm unsure how to invoke these tests. Is there a script to run through these?

I figured out how to run the tests locally.

quasicomputational · 2018-05-13T06:29:24Z

Cabal/ChangeLog.md

+    separate compilation of C++ source files to correctly build and link
+    non-library components (#5309).
+  * Reduced warnings generated by hsc2hs and c2hs when `cxx-options` field
+    is present in a omponent.


Typo: s/omponent/component/

Nice catch, I corrected this.

23Skidoo · 2018-05-17T08:15:15Z

There seem to be some genuine failures on AppVeyor.

quasicomputational · 2018-05-17T10:08:42Z

Cabal/Distribution/Simple/GHC.hs

@@ -657,14 +657,14 @@ buildOrReplLib forRepl verbosity numJobs pkg_descr lbi lib clbi = do
    info verbosity "Building C++ Sources..."
    sequence_
      [ do let baseCxxOpts    = Internal.componentCxxGhcOptions verbosity implInfo
-                               lbi libBi clbi libTargetDir filename
+                                lbi libBi clbi libTargetDir filename


Still a spurious whitespace change here

Don't we want the arguments to be vertically aligned the the function call above?

Yeah, sure. So long as it's deliberate and not just random reformatting noise.

quasicomputational · 2018-05-17T10:08:51Z

Cabal/Distribution/Simple/GHC.hs

-                               }
+                                  ghcOptProfilingMode = toFlag True,
+                                  ghcOptObjSuffix     = toFlag "p_o"
+                                }


And more whitespace changes here

Same deliberate, vertical alignment with this whitespace.

recursion-ninja · 2018-05-17T16:22:21Z

I added this test case, cabal-testsuite/PackageTests/Regression/T5309.

Unfortunately, I'm getting this error when running the test suite:

cabal-testsuite/PackageTests/Regression/T5309/cabal.test.hs                                                                       FAIL (0.03s)
$ /usr/local/bin/runghc -- '--ghc-arg=-i' '--ghc-arg=-no-user-package-db' '--ghc-arg=-package-db' '--ghc-arg=/home/washburn/.cabal/store/ghc-8.2.2/package.db' '--ghc-arg=-package-db' '--ghc-arg=/home/washburn/Code/cabal/dist-newstyle/packagedb/ghc-8.2.2' '--ghc-arg=-package-id' '--ghc-arg=Cabal-2.3.0.0-inplace' '--ghc-arg=-package-id' '--ghc-arg=async-2.1.1.1-H3j65XcXMHtBvmNwGCQ80G' '--ghc-arg=-package-id' '--ghc-arg=base-4.10.1.0' '--ghc-arg=-package-id' '--ghc-arg=cabal-testsuite-2.3.0.0-inplace' '--ghc-arg=-package-id' '--ghc-arg=clock-0.7.2-7c1444262a97961f79ec17849dbc10b53740bf835b4f1d79cf747e71b1d837f2' '--ghc-arg=-package-id' '--ghc-arg=exceptions-0.10.0-4a32c341144c5dc0a9ef4472b7272bfaf7e0cb4f64e97ebb952391bb7254d5fd' '--ghc-arg=-package-id' '--ghc-arg=filepath-1.4.1.2' '--ghc-arg=-package-id' '--ghc-arg=optparse-applicative-0.14.2.0-3dba83640de58524187465e129f9d0dd9ac3a9f1c5a8ae9afbf31e9b8e713aee' '--ghc-arg=-package-id' '--ghc-arg=process-1.6.1.0' '--ghc-arg=-package-id' '--ghc-arg=transformers-0.5.2.0' cabal-testsuite/PackageTests/Regression/T5309/cabal.test.hs --builddir /home/washburn/Code/cabal/dist-newstyle/build/x86_64-linux/ghc-8.2.2/cabal-testsuite-2.3.0.0 cabal-testsuite/PackageTests/Regression/T5309/cabal.test.hs
stdout:
# cabal new-build

stderr:
*** Exception: dieVerbatim: user error (<interactive>: The program 'cabal' is required but it could not be found.
)

*** unexpected failure for cabal-testsuite/PackageTests/Regression/T5309/cabal.test.hs

I'm not sure how to correct this? Any pointers @quasicomputational @23Skidoo

quasicomputational · 2018-05-17T16:35:41Z

I ran into that issue before; I think that it's a bug in setupAndCabalTest but I haven't poked around further. If you turn setupAndCabalTest into just cabalTest, I think it'll work---at least it did for me.

recursion-ninja · 2018-05-17T18:15:32Z

@quasicomputational I changed setupAndCabalTest to cabalTest and the test ran "successfully" but was marked as SKIP.

cabal-testsuite/PackageTests/Regression/T5309/cabal.test.hs                      SKIP (0.03s)

I'm guessing SKIP isn't what I want for the test case, I'd like it to try and build the local test project.

quasicomputational · 2018-05-17T18:29:32Z

Oh, run it (e: it = cabal-testsuite) with --with-cabal.

23Skidoo · 2018-05-17T19:31:21Z

cabal-testsuite/PackageTests/Regression/T5309/cabal.test.hs

This seems to be happening on master as well, so apparently not caused by this PR.

recursion-ninja · 2018-05-21T15:57:11Z

@23Skidoo I believe I corrected issues related to the IPIs test cases.

recursion-ninja · 2018-05-21T16:00:15Z

@quasicomputational, could you help me diagnose this error message? I'm still trying to add a stripped down version of this repository as a test case for the cxx-options & cxx-sources fields.

The error message is a bit incomprehensible to me. It seems to me that it's trying to install Cabal as a dependency, but I don't know why.

PackageTests\Regression\T5309\cabal.test.hs                                                                       FAIL (1.13s)
$ "C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\runghc.exe" "--" "--ghc-arg=-i" "--ghc-arg=-package-id" "--ghc-arg=Cabal-2.3.0.0-HTGg0qjuxXbBSwurLDB7ad" "--ghc-arg=-package-id" "--ghc-arg=async-2.2.1-FtyEBgS0La811OtEcI8hQO" "--ghc-arg=-package-id" "--ghc-arg=base-4.9.1.0" "--ghc-arg=-package-id" "--ghc-arg=cabal-testsuite-2.3.0.0-6oreefIYDsR8Jl4uC6dgsK" "--ghc-arg=-package-id" "--ghc-arg=clock-0.7.2-2aZP57zFattJce1nK6pmuj" "--ghc-arg=-package-id" "--ghc-arg=exceptions-0.10.0-D3l6tOLudZU9YCvtoPHr4p" "--ghc-arg=-package-id" "--ghc-arg=filepath-1.4.1.1" "--ghc-arg=-package-id" "--ghc-arg=optparse-applicative-0.14.2.0-8qLJq3LaLRK948X9OctCJT" "--ghc-arg=-package-id" "--ghc-arg=process-1.4.3.0" "--ghc-arg=-package-id" "--ghc-arg=transformers-0.5.2.0" "PackageTests\Regression\T5309\cabal.test.hs" "--builddir" "C:\projects\cabal\cabal-testsuite\dist" "PackageTests\Regression\T5309\cabal.test.hs" "--with-cabal" "..\cabal-install\dist\build\cabal\cabal.exe" "--skip-setup-tests"
stdout:
# cabal new-build
+ "C:\projects\cabal\cabal-testsuite\..\cabal-install\dist\build\cabal\cabal.exe" "new-build" "-vverbose +markoutput +nowrap" "--builddir" "C:\projects\cabal\cabal-testsuite\PackageTests\Regression\T5309\cabal.dist\work\.\dist" "--project-file" "cabal.project" "-j1" "all"
Project settings changed, reconfiguring...
creating C:\projects\cabal\cabal-testsuite\PackageTests\Regression\T5309\cabal.dist\work\dist
creating C:\projects\cabal\cabal-testsuite\PackageTests\Regression\T5309\cabal.dist\work
creating C:\projects\cabal\cabal-testsuite\PackageTests\Regression\T5309\cabal.dist\work\dist
creating C:\projects\cabal\cabal-testsuite\PackageTests\Regression\T5309\cabal.dist\work\dist\cache
this build was affected by the following (project) config files:
- C:\projects\cabal\cabal.project
Compiler settings changed, reconfiguring...
"C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc.exe" "--numeric-version"
looking for tool ghc-pkg near compiler in C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin
found ghc-pkg in C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc-pkg.exe
"C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc-pkg.exe" "--version"
"C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc.exe" "--supported-languages"
"C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc.exe" "--info"
"C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc.exe" "--print-global-package-db"
Warning: No remote package servers have been specified. Usually you would have one specified in the config file.
Failed to query pkg-config, Cabal will continue without solving for pkg-config constraints: dieVerbatim: user error (CallStack (from HasCallStack):
  die', called at .\Distribution\Simple\Program\Db.hs:423:28 in Cabal-2.3.0.0-HTGg0qjuxXbBSwurLDB7ad:Distribution.Simple.Program.Db
  requireProgram, called at .\Distribution\Solver\Types\PkgConfigDb.hs:65:21 in cabal-install-2.3.0.0-Cck6obqWNqJKjo9mr2iSL:Distribution.Solver.Types.PkgConfigDb
cabal.exe: The program 'pkg-config' is required but it could not be found.
)
-----BEGIN CABAL OUTPUT-----
Resolving dependencies...
-----END CABAL OUTPUT-----
CallStack (from HasCallStack):
  die', called at .\Distribution\Client\ProjectPlanning.hs:568:42 in cabal-install-2.3.0.0-Cck6obqWNqJKjo9mr2iSL:Distribution.Client.ProjectPlanning
-----BEGIN CABAL OUTPUT-----
cabal.exe: Could not resolve dependencies:
[__0] trying: Cabal-2.3.0.0 (user goal)
[__1] unknown package: text (dependency of Cabal)
[__1] fail (backjumping, conflict set: Cabal, text)
After searching the rest of the dependency tree exhaustively, these were the goals I've had most trouble fulfilling: Cabal (3), text (1)
-----END CABAL OUTPUT-----
stderr:
*** Exception: Command "C:\projects\cabal\cabal-testsuite\..\cabal-install\dist\build\cabal\cabal.exe" "new-build" "-vverbose +markoutput +nowrap" "--builddir" "C:\projects\cabal\cabal-testsuite\PackageTests\Regression\T5309\cabal.dist\work\.\dist" "--project-file" "cabal.project" "-j1" "all" failed.
Output:
Project settings changed, reconfiguring...
creating C:\projects\cabal\cabal-testsuite\PackageTests\Regression\T5309\cabal.dist\work\dist
creating C:\projects\cabal\cabal-testsuite\PackageTests\Regression\T5309\cabal.dist\work
creating C:\projects\cabal\cabal-testsuite\PackageTests\Regression\T5309\cabal.dist\work\dist
creating C:\projects\cabal\cabal-testsuite\PackageTests\Regression\T5309\cabal.dist\work\dist\cache
this build was affected by the following (project) config files:
- C:\projects\cabal\cabal.project
Compiler settings changed, reconfiguring...
"C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc.exe" "--numeric-version"
looking for tool ghc-pkg near compiler in C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin
found ghc-pkg in C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc-pkg.exe
"C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc-pkg.exe" "--version"
"C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc.exe" "--supported-languages"
"C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc.exe" "--info"
"C:\ProgramData\chocolatey\lib\ghc\tools\ghc-8.0.2\bin\ghc.exe" "--print-global-package-db"
Warning: No remote package servers have been specified. Usually you would have one specified in the config file.
Failed to query pkg-config, Cabal will continue without solving for pkg-config constraints: dieVerbatim: user error (CallStack (from HasCallStack):
  die', called at .\Distribution\Simple\Program\Db.hs:423:28 in Cabal-2.3.0.0-HTGg0qjuxXbBSwurLDB7ad:Distribution.Simple.Program.Db
  requireProgram, called at .\Distribution\Solver\Types\PkgConfigDb.hs:65:21 in cabal-install-2.3.0.0-Cck6obqWNqJKjo9mr2iSL:Distribution.Solver.Types.PkgConfigDb
cabal.exe: The program 'pkg-config' is required but it could not be found.
)
-----BEGIN CABAL OUTPUT-----
Resolving dependencies...
-----END CABAL OUTPUT-----
CallStack (from HasCallStack):
  die', called at .\Distribution\Client\ProjectPlanning.hs:568:42 in cabal-install-2.3.0.0-Cck6obqWNqJKjo9mr2iSL:Distribution.Client.ProjectPlanning
-----BEGIN CABAL OUTPUT-----
cabal.exe: Could not resolve dependencies:
[__0] trying: Cabal-2.3.0.0 (user goal)
[__1] unknown package: text (dependency of Cabal)
[__1] fail (backjumping, conflict set: Cabal, text)
After searching the rest of the dependency tree exhaustively, these were the goals I've had most trouble fulfilling: Cabal (3), text (1)
-----END CABAL OUTPUT-----
*** unexpected failure for PackageTests\Regression\T5309\cabal.test.hs

recursion-ninja · 2018-05-23T13:57:50Z

I fixed the T5309 test case and the IPIS test cases. AppVeyor is passing. Travis is not. I'm not sure if the Travis failures are things that need to be addressed. @quasicomputational , @23Skidoo could you inform me if there are any more test cases that need to be remedied?

For example, this Travis job failure doesn't look like something that needs to be fixed. It appears to be failing because files were added between two git commits.

However, it kooks like most of the failures involve authentication failures via SSH keys.

quasicomputational · 2018-05-23T14:50:04Z

Travis is broken at the moment, yeah. Should be fixed soon and then you can rebase on master and try again.

A couple of things:

Can the test case be cut down any more? It's about 2/3 of the diff at the moment, and it's taking 35-40s to run, which is a lot for a single test case.
The meta check is failing because you need to run make gen-extra-source-files.
I dunno what anyone else's opinion is here, but the PR's commit history is a bit messy with the merges. This isn't a requirement for merging, but if you've got spare cycles, maybe rebase and tify it up and squash a bit?

recursion-ninja · 2018-05-23T18:16:53Z

@quasicomputational I'll try and cut down the test suite a bit, but it does need to do a lot of things to test that the functionality is correct:

Require C sources via c-sources
Require C++ sources via cxx-sources
Have the C++ sources depend on some C sources
Have some C sources depend on some C++ sources
Build a library with C & C++ sources with different compiler options via cc-options and cxx-options
Build an executable from scratch with C & C++ sources with different compiler options via cc-options and cxx-options
Build an executable which depends on a library with C & C++ sources with different compiler options via cc-options and cxx-options
Build a benchmark from scratch with C & C++ sources with different compiler options via cc-options and cxx-options
Build a benchmark which depends on a library with C & C++ sources with different compiler options via cc-options and cxx-options
Build a test suite from scratch with C & C++ sources with different compiler options via cc-options and cxx-options
Build a test suite which depends on a library with C & C++ sources with different compiler options via cc-options and cxx-options

Since all the main functions are all stubs, I can try and remove some of the C & C++ code's functionality, as it's never invoked. Aside from that, I'm not sure how else to pare down the test suite.

I'll look into squashing some of the commits, but after I try and truncate the C & C++ code.

quasicomputational · 2018-05-23T18:22:16Z

Stubbing out the functionality is what I had in mind. Thanks a bunch!

quasicomputational · 2018-05-24T12:49:49Z

HEAD has the Travis stuff fixed now, so you probably want to rebase. Edit: Oh, it's been merged in. Carry on!

recursion-ninja · 2018-05-24T12:55:46Z

The test suite has been pared down, however the run time didn't noticeably decrease.

The make command has been run and the meta check passes.

All of the test suite tinkering commits have been squashed together.

@quasicomputational, I believe that this is ready to be merged. Should we wait until Travis is fixed on master and test things one last time with those changes before merging back into master?

quasicomputational · 2018-05-24T13:14:17Z

Cabal/Distribution/Simple/GHC.hs

-                       filter (/= mainModName) (exeModules exe))
+             return BuildSources {
+                        cSourcesFiles      = cSources bnfo,
+                        cxxSourceFiles    = cxxSources bnfo,


Code formatting here: equals aren't aligned.

quasicomputational · 2018-05-24T13:14:33Z

Cabal/Distribution/Simple/GHC.hs

-        else return (main : cSources bnfo, [], exeModules exe)
+          else return BuildSources {
+                          cSourcesFiles      = cSources bnfo,
+                          cxxSourceFiles    = cxxSources bnfo,


Also not aligned here

quasicomputational · 2018-05-24T13:23:05Z

Cabal/Distribution/Simple/GHC.hs

+                      }
+        else return BuildSources {
+                        cSourcesFiles      = main : cSources bnfo,
+                        cxxSourceFiles    = main : cxxSources bnfo,


These two lines don't look right. What if the main module is C++? (Are C++ main modules even supported?) What if it's in the C++-incompatible part of C? Don't we need to be able to tell between a C++ and a C module and then only add it to the appropriate list?

I wasn't anticipating supporting a C++ main module, only linking to C++ though the C ABI.

OK. So the main module shouldn't be added to cxxSourceFiles at all? This is definitely worth having a test for, with a main module that uses some C++-breaking construct.

I tried to address this by checking if main is a C++ source. If it is neither a Haskell source nor a C++ source file, then we default to C. Defaulting to C (appeared) to have been the previous functionality.

quasicomputational · 2018-05-24T13:26:47Z

Cabal/Distribution/Simple/GHC.hs

+              opts | needProfiling = profCxxOpts
+                   | needDynamic   = sharedCxxOpts
+                   | otherwise     = vanillaCxxOpts
+              odir = fromFlag (ghcOptObjDir opts)


Possibly stupid question: the C++ and C files are sharing an output directory here, so collisions are a possibility. Is that user error if it happens, or do we need to guard against it?

I would assume that collisions here is a user error. I also don't want the added complexity of a separate object directory for each (currently 2) FFI language.

OK. Might be an idea to warn about it (either here or in cabal check or both), but that can be done later.

I added a TODO for this potential collision issue. Should I create a GitHub issue for this so that it is visibly tracked.

quasicomputational · 2018-05-24T13:29:30Z

Cabal/Distribution/Simple/PreProcess.hs

+                                      -- interface and wrapped in a C source file.
+                                      -- Therefore we do not supply C++ flags
+                                      -- because there will not be C++ sources
+                                     {- ++ PD.cxxOptions   bi -} ]


Please remove the commented-out code.

quasicomputational · 2018-05-24T13:36:37Z

Cabal/Distribution/Simple/PreProcess.hs

@@ -525,7 +541,7 @@ getCppOptions bi lbi
    = platformDefines lbi
   ++ cppOptions bi
   ++ ["-I" ++ dir | dir <- PD.includeDirs bi]
-   ++ [opt | opt@('-':c:_) <- PD.ccOptions bi, c `elem` "DIU"]
+   ++ [opt | opt@('-':c:_) <- PD.ccOptions bi ++ PD.cxxOptions bi, c `elem` "DIU"]


This looks a bit messy: what if the C and C++ files need different CPP options? But we're already muddling up C and CPPed Haskell, so I guess this isn't a huge problem.

I have a vague recollection of something breaking without the cxx-options being added here, but that was over a month ago so I don't recall exactly what it was.

It does look like it works at the moment for the common case, and without this change C++ CPP options won't be plumbed through at all. I think the ideal solution here would be to have separate getCppOptionsForHs, getCppOptionsForCxx, getCppOptionsForC, etc functions, but, like I said, it's already a bit messy; I think it's sensible to leave that for later.

I agree that ideally this would be refactored out into separate functions.

However, I think leaving this for later would be best. I've already invested a lot of company time in this pull request and need to get back to implementing new features on our product.

I added a TODO above this function. Should I add a GitHub issue for this so that it is visibly tracked?

Yeah, that's fine. There's already a TODO about this, so let's open an issue and punt; it's evidently not causing anyone trouble today.

quasicomputational · 2018-05-24T13:37:03Z

Cabal/Distribution/Simple/PreProcess.hs

@@ -438,7 +445,8 @@ ppHsc2hs bi lbi clbi =
       ++ [ "--cflag=" ++ opt
          | pkg <- pkgs
          , opt <- [ "-I" ++ opt | opt <- Installed.includeDirs pkg ]
-                ++ [         opt | opt <- Installed.ccOptions   pkg ] ]
+                ++ [         opt | opt <- Installed.ccOptions   pkg
+                                      {- ++ Installed.cxxOptions  pkg -} ] ]


Commented out code again.

quasicomputational · 2018-05-24T13:37:17Z

Cabal/Distribution/Simple/PreProcess.hs

+                                                 -- Therefore we do not supply C++ flags
+                                                 -- because there will not be C++ sources
+                                                 --
+                                                 -- ++ Installed.cxxOptions pkg


And another bit of commented out code.

…incipled manner.

quasicomputational · 2018-05-24T15:05:33Z

Looks good to me. I've verified that the test case fails with cabal-install 2.2 and passes with this PR, and I don't see any potential show-stoppers.

recursion-ninja · 2018-05-24T15:46:34Z

@quasicomputational, I merged upstream master into my fork to re-run the Travis tests before finalizing this pull request. An AppVeyor test failed after the merge from master, but it doesn't seem to involve any changes in this pull request. Do you think it's safe to merge and close out this pull request? I don't want to "break the build."

quasicomputational · 2018-05-24T16:21:27Z

That test's flaky; I've seen it randomly fail before. I'm currently failing at getting it not to be skipped locally, but, if Travis comes back green-ish, it should be good to go.

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

domenkozar · 2019-07-04T13:32:11Z

Using cxx-options: --std=c++11 with clang (on darwin) library and then a different package executable will fail to apply that flag with Cabal 2.4.0.1. I suppose that should also propagate?

recursion-ninja added 7 commits May 11, 2018 19:45

intermetiate merge with master

7bd9c89

Merge remote-tracking branch 'upstream/master' into HEAD

7787adc

Correcting missing linking objects from non-library components.

c1efb8d

Removing C++ flags getting passed to C compiler.

849d7ce

Correcting local variable name.

7396231

Correcting local variable name, again.

e4730ec

Not supplying C++ options to hsc2hs & c2hs to prevernt erroneous warn…

e5836e3

…ings.

recursion-ninja added type: bug Cabal: cmd/build Cabal: cmd/test Cabal: cmd/bench Cabal: tests/unit-tests cabal-install: tests/unit-tests Cabal: stanza/foreign-library cabal-testsuite labels May 12, 2018

recursion-ninja self-assigned this May 12, 2018

recursion-ninja requested review from 23Skidoo and hvr May 12, 2018 18:34

quasicomputational reviewed May 13, 2018

View reviewed changes

quasicomputational reviewed May 17, 2018

View reviewed changes

Updating changelog, Adding test case for issue #5309

1ca8d24

Adding extra sources to .cabal file

082efbe

quasicomputational reviewed May 24, 2018

View reviewed changes

recursion-ninja added 3 commits May 24, 2018 10:24

Correcting whitespace/alignment issues. Adding main source in more pr…

cd4e9a9

…incipled manner.

Adding TODOs regarding more robust build proceedures [skip ci]

3f8c55c

Merge remote-tracking branch 'upstream/master'

76e93d4

recursion-ninja merged commit cc5f025 into haskell:master May 25, 2018

Fix linking issue with cxx-options and cxx-sources fields in non-library components #5315

Fix linking issue with cxx-options and cxx-sources fields in non-library components #5315

Uh oh!

Conversation

recursion-ninja commented May 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Standard checklist

Changes

Specific fixes

Before we merge

Uh oh!

quasicomputational commented May 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

23Skidoo commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

recursion-ninja commented May 17, 2018

Uh oh!

quasicomputational commented May 17, 2018

Uh oh!

recursion-ninja commented May 17, 2018

Uh oh!

quasicomputational commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

23Skidoo commented May 17, 2018

Uh oh!

recursion-ninja commented May 21, 2018

Uh oh!

recursion-ninja commented May 21, 2018

Uh oh!

recursion-ninja commented May 23, 2018

Uh oh!

quasicomputational commented May 23, 2018

Uh oh!

recursion-ninja commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quasicomputational commented May 23, 2018

Uh oh!

quasicomputational commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

recursion-ninja commented May 24, 2018

Uh oh!

Fix linking issue with `cxx-options` and `cxx-sources` fields in non-library components #5315

Fix linking issue with `cxx-options` and `cxx-sources` fields in non-library components #5315

recursion-ninja commented May 12, 2018 •

edited

Loading

quasicomputational commented May 17, 2018 •

edited

Loading

recursion-ninja commented May 23, 2018 •

edited

Loading

quasicomputational commented May 24, 2018 •

edited

Loading