Implement {cmm.asm}-{options,sources} for real #6033

hvr · 2019-05-05T16:13:40Z

While those buildinfo fields were added to the parser some time
ago via 57d7f28 and
4a28765 that work was never completed
by implementing the necessary build/sdist logic in Cabal.

This commit remedies this oversight by implementing and wiring up the
missing build logic.

cc @erikd

Things that work now (i.e. that I tested manually; these should be turned into testsuite tests long-term):

cmm-sources and asm-sources are compiled (in a separate phase like c-sources are) and the resulting object files are linked into the respective component (i.e. like c-sources)
cabal sdist properly includes cmm-sources and asm-sources in the src-dist
cmm-options are passed (directly) to GHC when compiling cmm-sources
asm-options are passed (via -opta) to GHC when compiling asm-sources files
ghc-options are NOT passed to compilation of cmm-sources/asm-sources

angerman · 2019-05-06T02:54:48Z

oversight

🤔

23Skidoo · 2019-05-06T14:23:41Z

TIL about Draft PRs, nice feature.

hsyl20 · 2019-06-26T11:15:18Z

I've added some missing C-- bits to @hvr's patch: https://github.com/hsyl20/cabal/tree/hsyl20-asm-cmm-sources
Hopefully we could have this patch for GHC 8.8

hvr · 2019-06-26T12:20:57Z

@hsyl20 thank you! I'll look at your branch asap and will cherry-pick the delta into this PR if it looks good

angerman · 2019-06-26T14:35:06Z

🎉

hvr · 2019-06-26T21:45:49Z

@hsyl20 I tested the code a bit, and it works partly; specifically cmm-options are not passed to the ghc commandline when compiling the .cmm module; however, ghc-options: ... for the respective component are passed to the .cmm compilation

hsyl20 · 2019-06-26T23:06:06Z

Cabal/Distribution/Simple/GHC/Internal.hs

+      ghcOptPackages       = toNubListR $ mkGhcOptPackages clbi,
+      ghcOptOptimisation   = toGhcOptimisation (withOptimization lbi),
+      ghcOptDebugInfo      = toFlag (withDebugInfo lbi),
+      ghcOptExtra          = hcOptions GHC bi,


Oops I forgot to fix this while copy-pasting from the other cases: s/hcOptions GHC bi/cmmOptions bi/
I guess it should fix cmm-options passing.

yeah... this is the kind of bugs I was worried about... :-)

I've rebased and updated the PR to include your fixup suggestion; I'm currently testdriving this locally

I'm also going to un-DRAFT this PR to invite more code-review

While those buildinfo fields were added to the parser some time ago via 57d7f28 and 4a28765 that work was never completed by implementing the necessary build/sdist logic in Cabal. This commit remedies this oversight by implementing and wiring up the missing build logic. *WARNING* this commit is still very WIP; ASM works mostly; C-- support is still incomplete

Cabal/doc/developing-packages.rst

23Skidoo

Code looks good modulo one comment.

23Skidoo · 2019-06-30T01:15:57Z

Cabal/Distribution/Simple/Build.hs

+-- information.
+addExtraAsmSources :: BuildInfo -> [FilePath] -> BuildInfo
+addExtraAsmSources bi extras = bi { asmSources = new }
+  where new = Set.toList $ old `Set.union` exs


If I have both preprocessor-generated asm-sources and cmm-sources, won't this add the union of these to the LBI's asmSources?

Actually, this probably will break if I have any preprocessor-generated C files at all.

23Skidoo · 2019-06-30T01:22:57Z

Would be also nice to see a test case, would be even nicer if the test case had a custom preprocessor that generated .s and .cmm files.

hvr · 2019-07-01T16:45:16Z

Cabal/Distribution/Simple/Build.hs

@@ -216,7 +216,13 @@ buildComponent verbosity numJobs pkg_descr lbi suffixes
    setupMessage' verbosity "Building" (packageId pkg_descr)
      (componentLocalName clbi) (maybeComponentInstantiatedWith clbi)
    let libbi = libBuildInfo lib
-        lib' = lib { libBuildInfo = addExtraCxxSources (addExtraCSources libbi extras) extras }


@23Skidoo based on your observation, wasn't this line already broken? i.e. the same FilePaths (i.e. extras) being added to cxx and c sources?

Yes, looks like it.

While those buildinfo fields were added to the parser some time ago via 57d7f28 and 4a28765 that work was never completed by implementing the necessary build/sdist logic in Cabal. This commit remedies this oversight by implementing and wiring up the missing build logic. NOTE: This is a backport of (at time of writing) unfinished #6033 to the 3.0 branch Specifically preprocessor-generated asm/cmm-sources need more work. Co-authored-by: Sylvain Henry <sylvain@haskus.fr>

hvr · 2019-07-01T22:43:51Z

@23Skidoo as discussed I've backported the current state of this PR to the 3.0 branch via c5395b8 and this PR remains open for the express purpose to fix the outstanding issues with the preprocessor generated sources

phadej

There are no tests.

While those buildinfo fields were added to the parser some time ago via 57d7f28 and 4a28765 that work was never completed by implementing the necessary build/sdist logic in Cabal. This commit remedies this oversight by implementing and wiring up the missing build logic. NOTE: This is a backport of (at time of writing) unfinished haskell#6033 to the 3.0 branch Specifically preprocessor-generated asm/cmm-sources need more work. Co-authored-by: Sylvain Henry <sylvain@haskus.fr>

phadej · 2019-11-28T02:26:03Z

#6376 merged, #6377 is tracking leftovers

These are only supported properly starting with cabal-version:3.0 (which has been enforced by Hackage as well -- via this very patch). See #6033 for details

These are only supported properly starting with cabal-version:3.0 (which has been enforced by Hackage as well -- via this very patch). See haskell#6033 for details

hvr added type: bug priority: high 🔥 labels May 5, 2019

hvr added this to the 3.0 milestone May 5, 2019

phadej mentioned this pull request Jun 10, 2019

Release for GHC 8.8 #5819

Closed

hvr force-pushed the pr/fix-asm-sources branch from 9b7299a to 15f068c Compare June 26, 2019 12:31

hsyl20 reviewed Jun 26, 2019

View reviewed changes

hvr force-pushed the pr/fix-asm-sources branch from 15f068c to c00c122 Compare June 27, 2019 06:59

hvr marked this pull request as ready for review June 27, 2019 07:02

Add C-- support bits for cmm-sources feature

e222728

hvr force-pushed the pr/fix-asm-sources branch from c00c122 to e222728 Compare June 27, 2019 07:06

hvr commented Jun 27, 2019

View reviewed changes

Cabal/doc/developing-packages.rst Show resolved Hide resolved

23Skidoo approved these changes Jun 30, 2019

View reviewed changes

hvr commented Jul 1, 2019

View reviewed changes

phadej suggested changes Jul 20, 2019

View reviewed changes

23Skidoo modified the milestones: 3.0, 3.0.1.0 Oct 16, 2019

This was referenced Nov 27, 2019

Pr/fix asm sources #6376

Merged

Leftovers from cmm-options fix #6377

Closed

phadej self-assigned this Nov 27, 2019

phadej closed this Nov 28, 2019

hvr deleted the pr/fix-asm-sources branch December 8, 2019 09:37

hvr added a commit that referenced this pull request Dec 8, 2019

Fix spec-ver lower-bound for some 3.0 fields in cabal check

b7cd00b

These are only supported properly starting with cabal-version:3.0 (which has been enforced by Hackage as well -- via this very patch). See #6033 for details

phadej mentioned this pull request Dec 13, 2019

Missing GHC 8.8.1 boot libraries haskell-infra/hackage-trustees#240

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement {cmm.asm}-{options,sources} for real #6033

Implement {cmm.asm}-{options,sources} for real #6033

hvr commented May 5, 2019 •

edited

Loading

angerman commented May 6, 2019

23Skidoo commented May 6, 2019

hsyl20 commented Jun 26, 2019

hvr commented Jun 26, 2019

angerman commented Jun 26, 2019

hvr commented Jun 26, 2019 •

edited

Loading

hsyl20 Jun 26, 2019

hvr Jun 27, 2019

23Skidoo left a comment

23Skidoo Jun 30, 2019 •

edited

Loading

23Skidoo Jun 30, 2019

23Skidoo commented Jun 30, 2019 •

edited

Loading

hvr Jul 1, 2019 •

edited

Loading

23Skidoo Jul 1, 2019

hvr commented Jul 1, 2019

phadej left a comment

phadej commented Nov 28, 2019

Implement {cmm.asm}-{options,sources} for real #6033

Implement {cmm.asm}-{options,sources} for real #6033

Conversation

hvr commented May 5, 2019 • edited Loading

angerman commented May 6, 2019

23Skidoo commented May 6, 2019

hsyl20 commented Jun 26, 2019

hvr commented Jun 26, 2019

angerman commented Jun 26, 2019

hvr commented Jun 26, 2019 • edited Loading

hsyl20 Jun 26, 2019

Choose a reason for hiding this comment

hvr Jun 27, 2019

Choose a reason for hiding this comment

23Skidoo left a comment

Choose a reason for hiding this comment

23Skidoo Jun 30, 2019 • edited Loading

Choose a reason for hiding this comment

23Skidoo Jun 30, 2019

Choose a reason for hiding this comment

23Skidoo commented Jun 30, 2019 • edited Loading

hvr Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

23Skidoo Jul 1, 2019

Choose a reason for hiding this comment

hvr commented Jul 1, 2019

phadej left a comment

Choose a reason for hiding this comment

phadej commented Nov 28, 2019

hvr commented May 5, 2019 •

edited

Loading

hvr commented Jun 26, 2019 •

edited

Loading

23Skidoo Jun 30, 2019 •

edited

Loading

23Skidoo commented Jun 30, 2019 •

edited

Loading

hvr Jul 1, 2019 •

edited

Loading