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.

Sign up for GitHub

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

Fixes To Target Traces #2016

Merged
merged 4 commits into from
Jul 8, 2018
Merged

Fixes To Target Traces #2016

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ffa1cb
Fix target results to show result based on the last target not all pr…
BlythMeister Jul 5, 2018
74e60c9
Tweak trace logic to allow target traces to pass an option
BlythMeister Jul 5, 2018
4482bbb
Fixes following code review
BlythMeister Jul 5, 2018
e993402
Tweaks to make the code look nicer
BlythMeister Jul 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/app/Fake.Core.Target/Target.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ and [<NoComparison>] [<NoEquality>] TargetContext =
member x.HasError =
x.PreviousTargets
|> List.exists (fun t -> t.Error.IsSome)
member x.LastTargetError =
x.PreviousTargets
|> List.last
Copy link
Member

Choose a reason for hiding this comment

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

strictly speaking LastTargetError needs to be a bool option as the PreviousTargets list might be empty...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh your so right...fixing now

|> fun x -> x.Error.IsSome
member x.TryFindPrevious name =
x.PreviousTargets |> List.tryFind (fun t -> t.Target.Name = name)
member x.TryFindTarget name =
Expand Down Expand Up @@ -318,9 +322,9 @@ module Target =
|> Seq.map (fun kv -> kv.Key)
|> Seq.fold (fun context name ->
let target = get name
use t = Trace.traceFinalTarget target.Name (match target.Description with Some d -> d | _ -> null) (dependencyString target)
use t = Trace.traceFinalTarget target.Name target.Description (dependencyString target)
let res = runSimpleContextInternal target context
if res.HasError
if res.LastTargetError
Copy link
Member

Choose a reason for hiding this comment

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

Another way would be to let runSimpleContextInternal return the state of the target it executed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooooh even nicer!

then t.MarkFailed()
else t.MarkSuccess()
res
Expand All @@ -334,9 +338,9 @@ module Target =
|> Seq.map (fun kv -> kv.Key)
|> Seq.fold (fun context name ->
let target = get name
use t = Trace.traceFailureTarget target.Name (match target.Description with Some d -> d | _ -> null) (dependencyString target)
use t = Trace.traceFailureTarget target.Name target.Description (dependencyString target)
let res = runSimpleContextInternal target context
if res.HasError
if res.LastTargetError
then t.MarkFailed()
else t.MarkSuccess()
res
Expand Down Expand Up @@ -506,9 +510,9 @@ module Target =
/// Runs a single target without its dependencies... only when no error has been detected yet.
let internal runSingleTarget (target : Target) (context:TargetContext) =
if not context.HasError then
use t = Trace.traceTarget target.Name (match target.Description with Some d -> d | _ -> null) (dependencyString target)
use t = Trace.traceTarget target.Name target.Description (dependencyString target)
let res = runSimpleContextInternal target context
if res.HasError
if res.LastTargetError
then t.MarkFailed()
else t.MarkSuccess()
res
Expand Down
20 changes: 13 additions & 7 deletions src/app/Fake.Core.Trace/Trace.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,11 @@ let traceHeader name =
traceLine()

/// Puts an opening tag on the internal tag stack
let openTagUnsafe tag (description:string) =
let openTagUnsafe tag description =
let sw = System.Diagnostics.Stopwatch.StartNew()
openTags.Value <- (sw, tag) :: openTags.Value
TraceData.OpenTag(tag, if System.String.IsNullOrEmpty description then None else Some description) |> CoreTracing.postMessage
let descriptionOption = if System.String.IsNullOrEmpty description then None else Some description
TraceData.OpenTag(tag, descriptionOption) |> CoreTracing.postMessage

type ISafeDisposable =
inherit System.IDisposable
Expand Down Expand Up @@ -225,20 +226,25 @@ let traceEndTargetUnsafe name =
[<System.Obsolete("Consider using traceTarget instead and 'use' to properly call traceEndTask in case of exceptions. To remove this warning use 'traceEndTargetUnsafe'.")>]
let traceEndTarget name = traceEndTargetUnsafe name

let private optionDescriptionToNullable description =
match description with
| Some d -> d
| _ -> null

let traceTarget name description dependencyString =
traceStartTargetUnsafe name description dependencyString
traceStartTargetUnsafe name (optionDescriptionToNullable description) dependencyString
Copy link
Member

Choose a reason for hiding this comment

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

But this changes the public API of traceTarget. We could argue that traceTarget is for internal use only but...

Copy link
Member

Choose a reason for hiding this comment

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

We could maybe add an traceTargetInternal and add a obsolete attribute stating "this feature is for internal use only". On the other hand others (like Xake for example) might want to use this API... So we should probably consider it public

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my argument 😆

I was also tempted to put the internal modifier, but then because of modules we can't since targets are not internal to trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i'll revert this then and have the narly logic in targets :(

asSafeDisposable (fun state -> traceEndTargetUnsafeEx state name)

let traceFinalTarget name description dependencyString =
traceStartFinalTargetUnsafe name description dependencyString
traceStartFinalTargetUnsafe name (optionDescriptionToNullable description) dependencyString
asSafeDisposable (fun state -> traceEndFinalTargetUnsafeEx state name)

let traceFailureTarget name description dependencyString =
traceStartFailureTargetUnsafe name description dependencyString
traceStartFailureTargetUnsafe name (optionDescriptionToNullable description) dependencyString
asSafeDisposable (fun state -> traceEndFailureTargetUnsafeEx state name)

/// Traces the begin of a task
let traceStartTaskUnsafe task (description:string) =
let traceStartTaskUnsafe task description =
openTagUnsafe (KnownTags.Task task) description

/// Traces the begin of a task
Expand All @@ -257,7 +263,7 @@ let traceEndTaskUnsafe task = traceEndTaskUnsafeEx TagStatus.Success task
let traceEndTask task = traceEndTaskUnsafe task

/// Wrap functions in a 'use' of this function
let traceTask name (description:string) =
let traceTask name description =
traceStartTaskUnsafe name description
asSafeDisposable (fun state -> traceEndTaskUnsafeEx state name)

Expand Down