Never use --enable-profiling when invoking Setup.

[ezyang]

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 #3790.

Signed-off-by: Edward Z. Yang [email protected]

[ezyang]

This may be worth backporting to 1.24, since it fixes a pretty major bug. @23Skidoo I'll let you decide.

[dcoutts]

@ezyang can we also check what we're doing in ProjectPlanning?

Seems like we ought to use this trick in there and only store the meaning, not the original packageConfigProf & packageConfigProfLib etc

(tryLibProfiling, tryExeProfiling) = computeEffectiveProfiling flags

So in the config file types we have

   packageConfigProf                :: Flag Bool, --TODO: [code cleanup] sort out
   packageConfigProfLib             :: Flag Bool, --      this duplication
   packageConfigProfExe             :: Flag Bool, --      and consistency

which is fine, and then in elaborateInstallPlan we have pkgsUseProfilingLibrary and related which are also trying to devine the semantics of those three input flags. So we should use your computeEffectiveProfiling at some point there and then store only the resulting lib + exe meaning (plus the downward closed stuff).

Looks like we currently ignore packageConfigProfExe entirely. Perhaps that's the "right thing" for the external UI, but you've looked at this issue in more (excruciating) detail, so you decide.

At least we are only storing lib + exe in the ElaboratedConfiguredPackage, so that aspect is ok at least.

   elabProfLib              :: Bool,
   elabProfExe              :: Bool,

[ezyang]

That is an orthogonal concern. I wanted SetupWrapper to do the right thing no matter who was calling it, with whatever ConfigFlags, so I think it makes sense for SetupWrapper to handle interpreting --enable-profiling appropriately so that anyone who uses this code will get the correct behavior (i.e. behavior consistent with the latest version).

Similarly, I think elabProfExe/elabProfLib are (1) semantically appropriate and (2) calculated in the correct way. In particular, effective profiling computation only tells you how a Setup script will interpret the various flags. It is totally inappropriate for doing closure. So I guess the only possible argument is that we should mark elabProfExe if profiling is enabled OR executable profiling is enabled. I suppose we could do that, but if we're going that far maybe we should just make it so that people can specify exactly what components they want profiled anyway (because per-component build lets us do that.)

[dcoutts]

That is an orthogonal concern.

Yes, fair enough.

@23Skidoo I'm happy to merge.

Similarly, I think elabProfExe/elabProfLib are (1) semantically appropriate and (2) calculated in the correct way.

Ok.

So I guess the only possible argument is that we should mark elabProfExe if profiling is enabled OR executable profiling is enabled.

Sounds reasonable. That's the settled UI behaviour for "classic build" in cabal-install-1.24 right?

I suppose we could do that, but if we're going that far maybe we should just make it so that people can specify exactly what components they want profiled anyway (because per-component build lets us do that.)

Currently they can do that per-package in the cabal.project file, just not per-component. We'd need to extend the project config system to allow talking about individual components and not just individual packages. That sounds like a generally useful thing (not special for profiling).

[ezyang]

Sounds reasonable. That's the settled UI behaviour for "classic build" in cabal-install-1.24 right?

Well, but in the code there's a comment saying that executable-profiling doesn't do anything because it is deprecated (which indeed it is). So I don't think it is a very strong argument.

Currently they can do that per-package in the cabal.project file, just not per-component. We'd need to extend the project config system to allow talking about individual components and not just individual packages. That sounds like a generally useful thing (not special for profiling).

Yes. But I am waiting for someone not-me to request it ;)

[dcoutts]

That sounds like a generally useful thing (not special for profiling).

Yes. But I am waiting for someone not-me to request it ;)

Right. Nice to have. More important things to do.

[23Skidoo]

See #3953 for the 1.24 version.