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

Profiling with Custom setup < 1.24 doesn't work #3790

Closed
ezyang opened this issue Sep 6, 2016 · 11 comments · Fixed by #3873
Closed

Profiling with Custom setup < 1.24 doesn't work #3790

ezyang opened this issue Sep 6, 2016 · 11 comments · Fixed by #3873

Comments

@ezyang
Copy link
Contributor

ezyang commented Sep 6, 2016

It seems that Cabal 1.22 doesn't understand how to install profiling objects, or maybe it doesn't understand the profiling flag? In any case, if we build a custom setup with Cabal 1.22 we will run into trouble if we try to do profiling; see @bitc's report at #3485 for where this cropped up in the wild.

One possible way to fix this is to add an extra solver parameter saying, "Want profiling" and then add extra Cabal library constraints (preferences?) accordingly. But I am a bit worried, because this means that the solver plan with and without profiling will vary, which is pretty confusing because you wouldn't expect turning on profiling to change what libraries get picked (you just wanted to profile the damn configuration you had!) So it's not altogether clear what we should do here. Maybe a compromise is to print a warning in this case?

@ezyang ezyang added this to the Later milestone Sep 6, 2016
@ezyang ezyang self-assigned this Sep 6, 2016
@dcoutts
Copy link
Contributor

dcoutts commented Sep 13, 2016

I believe this is due to the new --enable-profiling vs the old --enable-library-profiling vs --enable-executable-profiling flags and so it should be just a matter of translating into the older flags when talking to an older Cabal setup cli version.

What would be nice is if internally we translate from all these flags into one canonical value representing what it means, and the translate back when we construct the Setup.hs command line, and have that translation be version dependent.

@dcoutts
Copy link
Contributor

dcoutts commented Sep 13, 2016

Oh and just to record what we said elsewhere, that ideally we should not have to force cabal version constraints depending on feature flags because it's rather nice to have the property that the versions chosen does not depend on whether we're testing/profiling etc.

In cases where we know it's impossible to support a request to use some new feature with an older Cabal lib (used for a custom setup hs) then we can perhaps suggest that users use a flag (e.g. --constraint="Cabal >= x.y" or in cabal.project.local) so that it is explicit. Note that there's also the old cabal-lib-version flag, which the old code path uses to select the Cabal lib version for Setup.hs, which we could re-purpose as an alias for --constraint="Cabal == x.y".

@23Skidoo
Copy link
Member

I believe this is due to the new --enable-profiling vs the old --enable-library-profiling vs --enable-executable-profiling flags and so it should be just a matter of translating into the older flags when talking to an older Cabal setup cli version.

Hmm, I thought that we already did that?

/cc @ttuegel

@ezyang
Copy link
Contributor Author

ezyang commented Sep 13, 2016

Related #3502; right now constraint on Cabal is an all-or-nothing deal.

OK, so if we think we can support this for 1.22 that would solve the problem. I've changed the ticket name.

@ezyang ezyang changed the title Solver needs to force Cabal >= 1.24 for custom setup if profiling Profiling with Custom setup < 1.24 doesn't work; flags silently dropped (nix-local-build) Sep 13, 2016
@ezyang
Copy link
Contributor Author

ezyang commented Sep 18, 2016

Apparently we do that:

    -- Cabal < 1.21.1 doesn't know about 'disable-relocatable'
    -- Cabal < 1.21.1 doesn't know about 'enable-profiling'
    flags_1_21_1 =
      flags_1_22_0 { configRelocatable = NoFlag
                   , configProf = NoFlag
                   , configProfExe = configProf flags
                   , configProfLib =
                     mappend (configProf flags) (configProfLib flags)
                   , configCoverage = NoFlag
                   , configLibCoverage = configCoverage flags
                   }

So the 1.22 bug is different in some way.

@ezyang
Copy link
Contributor Author

ezyang commented Sep 18, 2016

Oh. Ohhhhhhh. mappend (Flag True) (Flag False) == Flag False. Oops. EDIT: Actually, this shouldn't cause the problem...

@ezyang
Copy link
Contributor Author

ezyang commented Sep 18, 2016

OK, so the semantics of --enable-profiling, --enable-library-profiling and --enable-executable-profiling have changed over the releases of Cabal. Here's what's going on:

HEAD (modestly abridged)

  let tryExeProfiling = fromFlagOrDefault False
                        (mappend (configProf cfg) (configProfExe cfg))
      tryLibProfiling = fromFlagOrDefault tryExeProfiling
                        (mappend (configProf cfg) (configProfLib cfg))

The mappend business is tricky, because if both flags are set, the later one is preferred. So here are some particularly tricky cases:

  • --enable-executable-profiling: lib YES, exe YES
  • --enable-profiling --disable-library-profiling: lib NO, exe YES
  • --disable-profiling --enable-executable-profiling: lib NO, exe YES

1.24.0.0

    let profEnabledBoth    = fromFlagOrDefault False (configProf cfg)
        profEnabledLib = fromFlagOrDefault profEnabledBoth (configProfLib cfg)
        profEnabledExe = fromFlagOrDefault profEnabledBoth (configProfExe cfg)

Hard cases:

  • --enable-executable-profiling: lib NO, exe YES (DIFFERENT!)
  • --enable-profiling --disable-library-profiling: lib NO, exe YES
  • --disable-profiling --enable-executable-profiling: lib NO, exe YES

1.22.0.0

        let withProfExe_ = fromFlagOrDefault False $ configProfExe cfg
            withProfLib_ = fromFlagOrDefault withProfExe_ $ configProfLib cfg

Hard case: --enable-executable-profiling: lib YES, exe YES (BACK TO HEAD BEHAVIOR!)

--enable-profiling was introduced in 1.24 which is when this stuff got complicated.

There is a hack in 1.22.5.0:

      ,option "" ["profiling"]
         "Executable profiling (requires library profiling)"
         -- HACK: See #2409. Thankfully, this is 1.22-specific.
         (\flags ->
           case (configProfLib flags, configProfExe flags) of
             (Flag a, Flag b)
               | (a == b)
               && ("cabalConfProf", "/TRUE") `elem` configProgramPaths flags
                    -> configProfExe flags
             _      -> NoFlag)
         (\v flags -> flags
               { configProfLib = v, configProfExe = v
               , configProgramPaths = ("cabalConfProf", "/TRUE")
               : configProgramPaths flags })
         (boolOpt [] [])

I... have no idea what this is.

1.20.0.0, 1.18.0 and probably everything older.

                    withProfLib         = fromFlag $ configProfLib cfg,
                    withProfExe         = fromFlag $ configProfExe cfg,

Hard case: --enable-library-profiling: lib NO, exe YES (AND IT'S BROKEN AGAIN)

So, I think what our setup wrapper script should do is just smooth over all the compatibility differences. EDIT: But none of these problems should be causing the bug reported here.

CC @ttuegel

@ezyang
Copy link
Contributor Author

ezyang commented Sep 18, 2016

OK, I've diagnosed the actual problem.

The problem is with 1.22.5.0 (and any other version with the hack mentioned in the comment above), the semantics of --disable-profiling/--enable-profiling depend on ordering (because the hack operates by looking at the current flag assignment and doing something). In particular, if I specify --enable-library-profiling --disable-profiling, I end up with library profiling DISABLED. This is NOT the case for 1.24, which has an order-invariant approach. If I swap the ordering things are OK.

@ezyang ezyang changed the title Profiling with Custom setup < 1.24 doesn't work; flags silently dropped (nix-local-build) Profiling with Custom setup < 1.24 doesn't work Sep 19, 2016
@ezyang
Copy link
Contributor Author

ezyang commented Sep 19, 2016

So, I think my suggested fix is that we NEVER pass --enable-profiling or --disable-profiling to Cabal. At the moment, and according to my historical analysis, Cabal ONLY uses configProf to affect the effective library/executable profiling, which means that anything we do with --enable-profiling, we can do using the library/executable profiling individually. Since these are always flags for the versions of Cabal library we support, we will get order invariance. Historical versions have varied on whether or not setting executable profiling implies library profiling, but if we set both explicitly this change in behavior doesn't matter. I'll code it up and test.

@ttuegel
Copy link
Member

ttuegel commented Sep 19, 2016

So, I think my suggested fix is that we NEVER pass --enable-profiling or --disable-profiling to Cabal.

That should be fine. Those flags exist to stop folks from shooting themselves in the foot by doing --enable-executable-profiling without --enable-library-profiling.

ezyang added a commit to ezyang/cabal that referenced this issue Sep 19, 2016
In Cabal 1.22.5.0, the semantics of
--disable-profiling/--enable-profiling depend on ordering (because there
is a hack that operates by looking at the current flag assignment and
doing something). In particular, if I specify --enable-library-profiling
--disable-profiling, I end up with library profiling DISABLED.

The fix is that we NEVER pass --enable-profiling or --disable-profiling
to Cabal. At the moment, and according to my historical analysis, Cabal
ONLY uses configProf to affect the effective library/executable
profiling, which means that anything we do with --enable-profiling, we
can do using the library/executable profiling individually. Since these
are always flags for the versions of Cabal library we support, we will
get order invariance. Historical versions have varied on whether or not
setting executable profiling implies library profiling, but if we set
both explicitly this change in behavior doesn't matter.

This patch is difficult to test because the bad profiling flags
can't be induced on an inplace build.  I tested by hand by building
a package that depended on 'distributive' by hand.

Fixes haskell#3790.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
@23Skidoo
Copy link
Member

23Skidoo commented Oct 8, 2016

Also fixed on the 1.24 branch in #3953.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants