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

Improve warning for old versions of HPC #1155

Merged
merged 1 commit into from Jan 1, 2013

Conversation

Projects
None yet
4 participants
@bmillwood
Copy link
Contributor

bmillwood commented Dec 18, 2012

I'm still not sure we want to go ahead with the hpc invocation at all if we are discarding search paths, but if we do, then we ought at least warn that we are doing so.

@ttuegel: as the author of bf2fb86, if you could comment a bit on whether or not hpc < 0.7 is really even usable, and under what circumstances it's useful, I'll add another commit to this pull request adding comments to the code. Particularly, how likely is it that hpc will still be useful after we discard some of its search paths?

@ttuegel

This comment has been minimized.

Copy link
Member

ttuegel commented Dec 18, 2012

@benmachine HPC will work for test suites that have their sources in a directory not shared with the library itself. This causes the test to be linked with the library, as is correct. Because Cabal only runs coverage for the library, it only needs the .mix files from when the library was built; hence this scenario would generate coverage correctly.
On the other hand, if the test and library sources share a directory, GHC will build the library into the test, so we need the .mix files from both the library and the test compilations. This is the scenario the patch you referenced fixes.

It should be noted that, given how Cabal invokes GHC, the only correct configuration is to have each package component in a separate subdirectory. To date, however, package authors have been noncompliant with this undocumented requirement :)

@ttuegel

This comment has been minimized.

Copy link
Member

ttuegel commented Dec 18, 2012

@benmachine I forgot to mention: if we abandon HPC invocations where search paths will be dropped, it will break coverage with every released version of GHC.

@bmillwood

This comment has been minimized.

Copy link
Contributor

bmillwood commented Dec 18, 2012

@ttuegel Thanks for the explanation. Do we have a guarantee that the first search path is the "most important" one? Would it be different if we dropped all but, say, the last one? If so, that's worth me including in my comments as well, maybe even in the Haddocks.

@feuerbach

This comment has been minimized.

Copy link
Contributor

feuerbach commented Dec 18, 2012

It should be noted that, given how Cabal invokes GHC, the only correct configuration is to have each package component in a separate subdirectory.

But doesn't this prohibit white-box tests (i.e. tests of internal, unexposed, components)? That doesn't sound right.

@tibbe

This comment has been minimized.

Copy link
Member

tibbe commented Dec 18, 2012

@ttuegel Can I delegate the review and merging of this patch to you? You understand this much better than I.

@ttuegel

This comment has been minimized.

Copy link
Member

ttuegel commented Dec 18, 2012

@tibbe Sure, no problem!

@ttuegel

This comment has been minimized.

Copy link
Member

ttuegel commented Dec 18, 2012

@feuerbach Sorry, I should have been clearer: Cabal should support the configuration where libraries are built into tests, benchmarks, or executables. However, because of the long-standing bug in how GHC is invoked, "white-boxing" doesn't always work right. There used to be a ticket for this, but I can't find it since the issue tracker was imported from Trac. Most of the problems were with supporting tools like HPC, but I think those problems have been solved. The one outstanding problem I have is that it isn't obvious to developers how to get white-box vs. black-box behavior, i.e., libraries rebuilt into executables vs. executables linked (statically or dynamically) to libraries. Most commonly, I've seen developers rebuilding when they want to be linking, and because GHC is static-linking by default, most of them don't even realized this is happening.

The point I was making here is that with GHC < 7.8, coverage only works "black-boxed" tests (but it does work!). GHC 7.8 should ship with a newer HPC that allows us to support "white-boxed" tests also. There is really no way to get white-boxed tests to work with the older HPC, though.

@ttuegel

This comment has been minimized.

Copy link
Member

ttuegel commented Dec 18, 2012

@benmachine With old HPC versions, if we use the library's .mix file search path, black-box tests will continue to work and white-box tests will continue to fail. (This was the status quo when I wrote the original patch.) You could reorder the paths and this would fix white-box tests, but black-box tests would fail.

With new versions of HPC, the paths will be searched in the order specified, but if more than one .mix file in any of these paths matches a particular module, HPC will consider it an error condition and die. This is OK, though, because it's matching not only module name, but a compilation time stamp and a hash, so if two different .mix files match the same module, something has gone terribly wrong.

@ttuegel

View changes

Cabal/Distribution/Simple/Program/Hpc.hs Outdated
versionMsg = maybe "" (\v -> " (Found HPC " ++ display v ++ ")")
(programVersion hpc)
(hpcDirs', msg) = case programVersion hpc of
Nothing -> (hpcDirs, Just $ "Could not determine HPC version: "

This comment has been minimized.

@ttuegel

ttuegel Dec 18, 2012

Member

I think it may be worth commenting (in the code, not the warning message) on what will happen if the optimistic assumption is wrong: HPC will take the list of search paths and apply them sequentially, each one replacing the last. So, it will search only the last path specified. This will break black-box tests, but only if HPC doesn't report its version correctly. As far as I know, all versions of HPC correctly report their version, so this should never happen.

This comment has been minimized.

@bmillwood

bmillwood Dec 18, 2012

Contributor

Will do.

@feuerbach

This comment has been minimized.

Copy link
Contributor

feuerbach commented Dec 18, 2012

@ttuegel I see — thanks a lot for clarifying this!

@bmillwood

This comment has been minimized.

Copy link
Contributor

bmillwood commented Dec 18, 2012

@ttuegel: ok, so how about we switch the behaviour from taking the first path in the case of pre-0.7 to taking the last path? Then it coincides with the behaviour in the case we guess the version wrongly, so there's only one failure mode, and (if I've understood you correctly) it fixes white-box tests at the expense of black-box tests – but all black box tests can be implemented, at some efficiency cost, as white-box tests, right? And the reverse is not in general true.

How does that sound?

@bmillwood

This comment has been minimized.

Copy link
Contributor

bmillwood commented Dec 18, 2012

In fact, why don't we just pass all the paths to hpc in every case, but document (and warn) when the version is such that they will all be ignored but the last one?

@ttuegel

This comment has been minimized.

Copy link
Member

ttuegel commented Dec 19, 2012

@benmachine I we should keep the same behavior that we have now for old versions of HPC. From what I've seen, there are a relatively small number of people wanting to use white box tests compared to the number of people successfully using black box tests, and I think it's easier to tell the former to upgrade than to tell the latter to upgrade or rewrite their test suites. To put it another way, since we can't fix the problem outright, I think we ought to err on the side of consistency. (This is all probably moot anyway, because likely >90% of people won't receive this update to Cabal until they upgrade GHC.)

@feuerbach

This comment has been minimized.

Copy link
Contributor

feuerbach commented Dec 19, 2012

@ttuegel could you point me to the changes in GHC that you mentioned? I assume they should be in either utils/hpc or libraries/hpc, but I don't see anything relevant.

Or have they not been merged yet?

(Sorry for the off-topic noise here — don't know a better place to ask.)

@ttuegel

This comment has been minimized.

Copy link
Member

ttuegel commented Dec 20, 2012

@feuerbach The only change (modulo some documentation and the version) is to utils/hpc/HpcFlags.hs. There is a ticket on the GHC Trac.

@bmillwood

This comment has been minimized.

Copy link
Contributor

bmillwood commented Dec 22, 2012

@ttuegel ok, one last proposal: is it okay to just die with an error message if the version of hpc could not be found, since there is no known scenario in which that happens?

@ttuegel

This comment has been minimized.

Copy link
Member

ttuegel commented Dec 22, 2012

@benmachine Yes, I think that would be fine. All the versions of HPC I know of correctly report their version information; if someone has a version that old, they need to upgrade, anyway. I would make sure the error message still indicates what version we are looking for, so that hopefully they realize their HPC is out of date.

Improve warning for old versions of HPC
Now the warning message includes mention of the input data that is being
ignored because hpc can't deal with it.

This involves a change of behaviour: when the HPC version cannot be
determined, this is now a fatal error, rather than just assuming an old
version.
@bmillwood

This comment has been minimized.

Copy link
Contributor

bmillwood commented Dec 22, 2012

@ttuegel ok, I just did a forced update to replace the old commit with this new one. The behaviour in th case of a missing version is in line with the behaviour of other programs, since that's now done by requireProgramVersion.

Any further comments?

@bmillwood

This comment has been minimized.

Copy link
Contributor

bmillwood commented Dec 31, 2012

As an aside, code style issue: I pass the version of hpc from requireProgramVersion because I think it's nicer than doing an irrefutable pattern-match on programVersion hpc, but this does mean that the same information is passed to the function twice, violating DRY. Any opinions?

@ttuegel

This comment has been minimized.

Copy link
Member

ttuegel commented Jan 1, 2013

@benmachine Sounds good! The error message is greatly improved and I'm satisfied that Cabal's actual behavior is still unchanged.

I am not concerned much about the repetition because it existed before your patch and fixing it would involve more far-reaching changes. (I agree that we want to avoid the irrefutable pattern match. To truly avoid the repetition, we could move the call to requireProgramVersion into markup itself. To remove the call from markupPackage, though, we would have to make the same change to union.)

ttuegel added a commit that referenced this pull request Jan 1, 2013

Merge pull request #1155 from benmachine/hpcmsg
Improve warning for old versions of HPC

@ttuegel ttuegel merged commit 037fed7 into haskell:master Jan 1, 2013

@bmillwood bmillwood deleted the bmillwood:hpcmsg branch Jan 2, 2013

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