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

Include a prof subdirectory in dist-dir layout #5314

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@cocreature
Copy link
Collaborator

cocreature commented May 10, 2018

Switch between profiling and non-profiling builds is a fairly common
usecase so supporting this without forcing a recompile is pretty
useful.

Please include the following checklist in your PR:

  • 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 tested this both on a very simple project as well as a large project with a lot of vendored deps, where this makes a huge difference.

@hvr mentioned that eventually it might be nice to split prof/non-prof into separate units but that this is a useful low-hanging fruit in the meantime.

Include a prof subdirectory in dist-dir layout
Switch between profiling and non-profiling builds is a fairly common
usecase so supporting this without forcing a recompile is pretty
useful.
@@ -434,7 +434,8 @@ elabDistDirParams shared elab = DistDirParams {
ElabPackage _ -> Nothing,
distParamCompilerId = compilerId (pkgConfigCompiler shared),
distParamPlatform = pkgConfigPlatform shared,
distParamOptimization = elabOptimization elab
distParamOptimization = elabOptimization elab,
distParamProfiling = elabProfLib elab || elabProfExe elab

This comment has been minimized.

Copy link
@cocreature

cocreature May 10, 2018

Author Collaborator

I am not sure if this is the proper way to check this. If anyone has a better suggestion, I’m all ears.

This comment has been minimized.

Copy link
@dcoutts

dcoutts May 29, 2018

Member

Ok, here's the tricky thing...

If we are dealing with standard packages where we can split them up and handle the components individually (ie elabPkgOrComp elab is a ElabComponent _) then I think this approach should work ok, since each component is either being built with profiling or without, but not a mixture.

On the other hand if we're dealing with a multi-component package that we have to handle as a monolit (ie elabPkgOrComp elab is a ElabPackage _) then I think we have a problem because different components within that package can be built for profiling or not. For example we can enable both the profiling and non-profiling flavours of libs, and just non-profiling of an exe in the same package. It's worth remembering that the --enable-library-profiling and flag adds additional artefacts, it doesn't switch the flavour of a single set of artefacts.

@alexbiehl

This comment has been minimized.

Copy link
Member

alexbiehl commented May 29, 2018

Can we have this? I am profiling a lot lately and I am tripping this quite often. @23Skidoo

@dcoutts
Copy link
Member

dcoutts left a comment

I'm not yet sure I see how this works.

A package or indeed a component is not a thing that is built one of the ways: vanilla | dynamic | profiling. Rather it can be built and/all of those ways, and all the artefacts will be built.

As you allude to, the "proper" way to do this will be to duplicate the components in the component graph and enable a single flavour in each one, giving each one their own local build dir or store id.

In the meantime, short of doing that, you're proposing this scheme, but it's not immediately clear how it behaves given the "any/all flavour" behaviour. Perhaps you can elaborate.

@@ -434,7 +434,8 @@ elabDistDirParams shared elab = DistDirParams {
ElabPackage _ -> Nothing,
distParamCompilerId = compilerId (pkgConfigCompiler shared),
distParamPlatform = pkgConfigPlatform shared,
distParamOptimization = elabOptimization elab
distParamOptimization = elabOptimization elab,
distParamProfiling = elabProfLib elab || elabProfExe elab

This comment has been minimized.

Copy link
@dcoutts

dcoutts May 29, 2018

Member

Ok, here's the tricky thing...

If we are dealing with standard packages where we can split them up and handle the components individually (ie elabPkgOrComp elab is a ElabComponent _) then I think this approach should work ok, since each component is either being built with profiling or without, but not a mixture.

On the other hand if we're dealing with a multi-component package that we have to handle as a monolit (ie elabPkgOrComp elab is a ElabPackage _) then I think we have a problem because different components within that package can be built for profiling or not. For example we can enable both the profiling and non-profiling flavours of libs, and just non-profiling of an exe in the same package. It's worth remembering that the --enable-library-profiling and flag adds additional artefacts, it doesn't switch the flavour of a single set of artefacts.

@edsko

This comment has been minimized.

Copy link
Contributor

edsko commented Jun 5, 2018

At @dcoutts 's request I just gave this a spin on a rather large project, with quite a large number of local packages, a ton of constraints to pin it to a particular Stackage version (basically, I manually translated a stack.yaml file to a cabal.project file). I'd love to be able to switch back and forth between profiling and non-profiling builds because at the moment this incurs a looooong rebuild. But am getting some unexpected results:

# rm -r dist-newstyle
# cabal new-build wallet-unit-tests
... works
# cabal new-build --enable-profiling wallet-unit-tests
... works
... now make a small (unrelated) modification to one of the source files in the `wallet-unit-tests` test suite
# cabal new-build wallet-unit-tests

test/unit/Util/Validated.hs:58:47: error:
    • Couldn't match expected type ‘Formatting.Internal.Format
                                      (a1 -> Data.Text.Internal.Builder.Builder)
                                      ([Text] -> e -> Data.Text.Internal.Builder.Builder)’
                  with actual type ‘formatting-6.2.4@formatting-6.2.4-9f1a04794165ec1db112b9a353fecf58e9ace202d0c3b964e4f691c853b2b287:Formatting.Internal.Format
                                      r0 (t0 a0 -> r0)’
      NB: ‘formatting-6.2.4@formatting-6.2.4-9f1a04794165ec1db112b9a353fecf58e9ace202d0c3b964e4f691c853b2b287:Formatting.Internal.Format’
            is defined in ‘Formatting.Internal’
                in package ‘formatting-6.2.4@formatting-6.2.4-9f1a04794165ec1db112b9a353fecf58e9ace202d0c3b964e4f691c853b2b287’
          ‘Formatting.Internal.Format’
            is defined in ‘Formatting.Internal’
                in package ‘formatting-6.2.4@formatting-6.2.4-6b4832019a3152a88f19d2b0c435e554cd236994817439abfc5e52503343f14e’

So it seems something somewhere is getting confused.

@cocreature

This comment has been minimized.

Copy link
Collaborator Author

cocreature commented Jun 5, 2018

I don’t know when I will have time to work on this again so if someone else wants to pick this up, be my guest.

@edsko Do you have some public testcase for this? I just tried it again and it seems to work fine for me

@dcoutts So I’m not too familiar with the cabal internals so I’m just documenting how I think this should work, feel free to correct me: So currently all artifacts end up in the same directory. This forces a rebuild when switching between non-profiling and profiling builds. This patch adds a new subdirectory to the already existing subdirectories (e.g. the opt subdirectory). This directory is used if at least one of library profiling and executable profiling is enabled. This will mean that you still get a rebuild in some cases, e.g., if you only disable executable profiling while leaving on library profiling since the prof subdirectory will still be used. But a) I don’t think that is that common of a thing to do and b) you also get a rebuild in that case atm so it’s not a regression.

@edsko

This comment has been minimized.

Copy link
Contributor

edsko commented Jun 25, 2018

I do have a public test case for you, but until now it was a bit awkward to reproduce. However, I've now written a (first, very preliminary) version of https://github.com/edsko/stack2cabal , which is making this a lot easier. These are the steps to that should reproduce the above problem:

  1. Check out the develop branch of https://github.com/input-output-hk/cardano-sl/ (I was using commit f2877d00dc15ff644e13f1a0bc56b45c07628c13).
  2. Use my stack2cabal tool to checkout all external dependencies using stack2cabal -c stack.yaml
  3. Use stack2cabal again to construct a cabal.project file using stack2cabal -w stack.yaml
  4. Run cabal new-build wallet-unit-tests
  5. Run cabal new-build --enable-profiling wallet-unit-tests
  6. Make a small change to, say, wallet-new/test/unit/WalletUnitTest.hs
  7. Run cabal new-build wallet-unit-tests again

However, as I was experimenting with this, I think I happened to come across an unrelated bug in the prof-dir branch also. Step (5) failed when trying to build byteable. I think it compiles and links just fine, but then when it tries to copy it to the store, that fails with

Using internal setup method with build-type Simple and args:
["copy","--verbose=2","--builddir=dist","--destdir=/home/edsko/.cabal/store/ghc-8.2.2/incoming/new-30416"]
getDirectoryContents:openDirStream: does not exist (No such file or directory)

It doesn't throw this same error for any other package, strangely. I could only complete the build if I run new-build using standard cabal, and then running cabal new-build using the prof-dir branch again after byteable was now available in the store. Haven't tried reproducing this a second time by emptying my store, but I suspect that ought to be possible.

Even after that, when I run step (7), it starts building a ton of additional stuff and then fails somewhere else again... this time with

gcc: error: /home/edsko/wt/clients/test/cardano-sl/dist-newstyle/build/x86_64-linux/ghc-8.2.2/cborg-0.2.0.0/build/Codec/CBOR/Read.dyn_o: No such file or directory

It seems there are still some issues with this branch. But maybe this will at least provide a larger example to play with.

@fommil

This comment was marked as outdated.

Copy link
Member

fommil commented Nov 16, 2018

Just wanted to say that @cocreature is my hero... I am going to trial this out next week.

@fommil

This comment was marked as outdated.

Copy link
Member

fommil commented Nov 16, 2018

I am getting some strange interactions with opts/noopts with this patch applied to the latest 2.4 branch. I haven't isolated the problem... it's hard to even reproduce, but I'm seeing big recompiles swapping between cabal new-build and cabal new-build -O0 without even considering --enable-profiling. I also had a bizarre situation where servant-server (a regular dependency) stopped compiling. cabal new-clean seemed to fix that.

@fommil

This comment was marked as outdated.

Copy link
Member

fommil commented Nov 16, 2018

Sorry for the noise, I think my problem might not be related to this patch... I'm changing a lot of things and I have just isolated that adding these custom ghc-options to a git source-dependency is confusing the hell out of cabal...

source-repository-package
    type: git
    location: https://github.com/symbiont-io/text-format.git
    tag: 142dc88bcd94bf2780f7c3f11095f4b529e80449

package text-format
  ghc-options: -Wno-dodgy-imports -Wno-unused-imports

so I'll try again next week and take what I said in the previous comment with a huge grain of salt.

@fommil

This comment has been minimized.

Copy link
Member

fommil commented Dec 20, 2018

I'm going to try this again in 2.4.0.1

@fommil

This comment has been minimized.

Copy link
Member

fommil commented Dec 20, 2018

ok, my build broke. I got a tonne of errors that looked like this

    • Couldn't match type ‘async-2.2.1:Control.Concurrent.Async.Async
                             ()’
                     with ‘Async a0’
      NB: ‘Async’
            is defined in ‘Control.Concurrent.Async’ in package ‘async-2.2.1’
          ‘async-2.2.1:Control.Concurrent.Async.Async’
            is defined in ‘Control.Concurrent.Async’ in package ‘async-2.2.1’
      Expected type: [async-2.2.1:Control.Concurrent.Async.Async ()]
                     -> IO ()
        Actual type: [Async a0] -> IO ()

when I swapped from cabal v2-run tasty foo to cabal v2-build all.

Now that I'm back on regular 2.4.0.1, and doing a git clean -xfd, I'm getting similar things just swapping between --enable-profiling -O1 and -O0 (no profiling). So perhaps the bug was there already? I guess I'm just saying I see the same thing as @edsko and I'm using -O1 / -O0 in the released cabal to try to simulate separate caches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.