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

LocalBuildInfo: combine profiling settings, track program coverage #3610

Merged
merged 2 commits into from
Jul 25, 2016

Conversation

ttuegel
Copy link
Member

@ttuegel ttuegel commented Jul 24, 2016

The profiling settings in LocalBuildInfo are combined as discussed in #3597. Fields were added to track if program coverage is enabled. Both features are now disabled (and a warning emitted) if the compiler does not support them.

@@ -301,3 +308,15 @@ externalPackageDeps lbi =
-- True if this dependency is an internal one (depends on the library
-- defined in the same package).
internal ipkgid = any ((==ipkgid) . componentUnitId) (Graph.toList (componentGraph lbi))

withProfLib :: LocalBuildInfo -> Bool
withProfLib = isJust . libProfiling
Copy link
Contributor

@ezyang ezyang Jul 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ide-backpend-0.10.0.1 is using this as a record setter http://hs01.scs.stanford.edu:8910/search/?q=withProfLib+%2B%3D

(Don't have to do anything, but maybe would be nice to give them a heads up.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezyang I was on the fence about renaming these accessors anyway because the compiler-specific build logic is very tightly integrated with the access pattern. I will just revert this part of the patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it does look like ide-backend is the ONLY client, so I think it's OK if you decide to rename.

@ezyang
Copy link
Contributor

ezyang commented Jul 24, 2016

I guess this is feeding into the eventual flags refactor, right? Looks fine.

@ttuegel
Copy link
Member Author

ttuegel commented Jul 24, 2016

I guess this is feeding into the eventual flags refactor, right? Looks fine.

These are the actual problems I found while I was working on the flags refactor.

The flags refactor is going to become a note in the documentation that says, "Hey, don't use mempty on the left side of <>!" The amount of effort to deprecate fromFlag isn't worth it.

@ezyang
Copy link
Contributor

ezyang commented Jul 24, 2016

That's too bad, but not that surprising :( Well, maybe there is some useful commentary you can add to the ticket.

@ttuegel ttuegel force-pushed the lbi-coverage branch 3 times, most recently from de4e22b to 990efe3 Compare July 24, 2016 20:25
++ "profiling. Profiling has been disabled.")
return (False, id)

when profEnabled $ warn verbosity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is this correct? Was: when (profEnabledExe && not profEnabledLib). Now: when (tryExeProfiling || tryLibProfiling).

@23Skidoo
Copy link
Member

Looks OK.

@ttuegel
Copy link
Member Author

ttuegel commented Jul 24, 2016

Quite right about that conditional, thanks!

ttuegel added 2 commits July 25, 2016 11:33
The status of Haskell program coverage is recorded in
LocalBuildInfo. Program coverage is disabled during configure if the
compiler does not support it.
@dcoutts
Copy link
Contributor

dcoutts commented Jul 25, 2016

Note that there's some hope of eliminating the fromFlag style since the new-build code hardly uses it at all iirc. We try instead to convert into other non-flag types and at that conversion boundary apply defaults etc.

@ttuegel ttuegel merged commit f9e7463 into haskell:master Jul 25, 2016
@ttuegel ttuegel deleted the lbi-coverage branch July 25, 2016 22:43
@ezyang ezyang modified the milestones: Cabal 2.0, 2.0 (planned for next feature release) Sep 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants