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

Trigger subsequent targets #119

Closed
werwolfby opened this issue Aug 14, 2018 · 15 comments
Closed

Trigger subsequent targets #119

werwolfby opened this issue Aug 14, 2018 · 15 comments
Milestone

Comments

@werwolfby
Copy link
Contributor

werwolfby commented Aug 14, 2018

I have 2 targets:

UpdateVersion - target that update version in AssemblyInfo.cs
CreateReleaseBranch - target that create release branch (I'm lazy to do it manually 😄)

I want after execute target CreateReleaseBranch execute UpdateVersion as well. So I can't add DependsOn(CreateReleaseBranch) for UpdateVersion target because I can call UpdateVersion sometimes and do not want to execute CreateReleaseBranch.

I understand that I can use -Skip to not execute CreateReleaseBranch when I do run UpdateVersion but this is ugly workaround because I can forget about this.

The only way I can manage right now is to create additional target CreateReleaseBranchAndUpdateVersion which has right dependencies and use Before(UpdateVersion) on CreateReleaseBranch. But I want something like this:

Target CreateReleaseBranch => _ => _
    .Executes(() =>
    {
        // Some staff
        UpdateVersion.Invoke();
        // or
        Execute(UpdateVersion);
    }

Because it doesn't make any sense to create release branch without updating version in develop branch.

Is it possible to do it?

If you can point me somewhere in source, I'll create a PR for this feature.

I think it can be useful in some cases.

@MarkusAmshove
Copy link
Contributor

At the moment you could create a simple C# method UpdateAssemblyInfo which you call within the Target CreateReleaseBranch and within the Target UpdateVersion, as you can use all tools within methods.

Adding something like .After(CreateReleaseBranch) or .AlwaysAfter(CreateReleaseBranch) would change the current flow of build files, so I guess this should be a bigger discussion.
On that topic, we could also think about adding virtual methods like BeforeAll and AfterAll (or BeforeBuild and AfterBuild or something else) to make it possible to hook into specific parts of the build.
But that would also be up for discussion

@MarkusAmshove
Copy link
Contributor

If you want to give it a shot, this should be the starting point of the build execution

@arodus
Copy link
Contributor

arodus commented Oct 23, 2018

@werwolfby did you already found a solution? I think it should be solvable with .Before and .After.

@werwolfby
Copy link
Contributor Author

@arodus No I didn't. And .Before and .After doesn't solve it. Because sometimes I need to call some target in the middle of another target.
And now I can do it only by extracting some method that will be used by both targets.

@ljani
Copy link

ljani commented Oct 25, 2018

If I understood correctly, I'm having a similar problem:

  • I have a target called RunPostgres (Docker container)
  • I have another target called Test, which depends on RunPostgres
    • This is the target I want to run
  • Finally I have a target called StopPostgres, which I would like to run after Test
    • Or better, at the end of build if RunPostgres was invoked from any target

While I can work around this using C#, because Nuke's design is clever, I'm intrigued how this should be solved with targets. In a traditional Makefile, one solution is to recursively call make at the end of Test as far as I know. But a few alternative options come to mind.

  1. Make the order of actions matter:
public Target Test => _ => _
    .DependsOn(RunPostgres)
    .Executes(() => { /* Execute test */ })
    .DependsOn(StopPostgres);
  1. Allow circular dependencies with After and Before:
public Target Test => _ => _
    .DependsOn(RunPostgres)
    .DependsOn(StopPostgres)
    .Before(StopPostgres)
    .Executes(() => { /* Execute test */ });
  1. Introduce eg. Finally:
public Target Test => _ => _
    .DependsOn(RunPostgres)
    .Executes(() => { /* Execute test */ })
    .Finally(StopPostgres);
  1. Ability to invoke StopPostgres from OnBuildFinished (see Trigger subsequent targets #119 (comment) for syntax).

EDIT: 5) Support Job Objects in a cross-platform way to kill children when a parent dies.

@arodus
Copy link
Contributor

arodus commented Oct 26, 2018

tl;dr:

  • Think outside of the normal build DSL pattern and also utilize C# as a host language
  • Breaking the dependency graph by executing a target from within another target or allowing circular references is a strong won't fix, as we think there are other ways of solving such issues
  • Defining an execution path the other way round is something we will think about

NUKE is designed to save the user from running into (WTF) situations, in which the execution of targets is no longer easily comprehendable. Calling targets from within other targets would make it impossible to construct a proper dependency graph and would break features like build.ps1 --graph.

Another reason why this implementation was chosen, is that nuke is a native c# application wich allows the users to solve nearly every problem in other ways. So, there is no reason to force everything in targets. For example in your case @ljani, I would use something like:

public Target TestPostgres => _ => _
    .Executes(() =>
    {
        using (CreatePostgresContainer())
        {
            //do work
        }
    });

private IDisposable CreatePostgresContainer() =>  DelegateDisposable.CreateBracket(StartPostgres, StopPostgres);

private void StartPostgres() { }
private void StopPostgres() { }

This would allow the usage of the postgres container everywhere and not just for a complete target.
You could also use it in combination with NukeBuild.OnBuildInitialized() and NukeBuildFinished() even.

We could introduce Target.Using(params IDisposable[]) which would help in this case. You could probably also add a variation of this yourself as extension method to your own code.

Make the order of actions matter:

DependsOn is defined as "Other target is required to run before this target". There are no assumptions based on the order in which targets are passed to this method.

Introduce eg. Finally

Since it would not break the dependency graph this is something we could think about. Would something like this also work for you @werwolfby ?

Support Job Objects

I never worked with them but will have a look into it.

@ljani
Copy link

ljani commented Oct 26, 2018

Regarding my poor ideas, yeah, I knew most of them were garbage. I was just brainstorming ways to solve this 😄 Something utilizing IDisposable is definitely an interesting idea and very csharpy. I really like eg. xunit how it does things.

So, there is no reason to force everything in targets.

Yes, my use case is a simple one as I'm still evaluating the tool 😄 The thing is that once you drop to C#, you'll lose the dependency graph. If StopPostgres (or DoComplexCleanup) depends on some other targets, those would not be called and I would need to build my own dependency graph for them. So, I think the whole point of having a build tool like Nuke is to prefer targets over functions to support the dependency graph.

I would use something like:

Yes, this is essentially what I referred as a workaround and I'm doing now.

Using, Finally, etc

I'm sure you noticed, but one thing to keep in mind is that multiple targets might depend on the same cleanup function.

I don't know if you have any experience with systemd, but some inspiration could be drawn from their dependency graph.

@matkoch
Copy link
Member

matkoch commented Oct 28, 2018

Target Setup => _ => _
   .Executes(() => {});

Target DoSomething => _ => _
   .DependsOn(Setup)
   .Triggers(Cleanup)
   .Executes(() => {});

Target Cleanup => _ => _
   .Executes(() => {});

Although I'm not sure how we can visualize this in --graph.

@matkoch matkoch changed the title Execute some target after other target Trigger subsequent targets Oct 30, 2018
@matkoch matkoch added this to the v0.14.0 milestone Oct 30, 2018
@matkoch matkoch modified the milestones: v0.17.0, v0.15.0 Jan 3, 2019
@matkoch matkoch closed this as completed Jan 3, 2019
@ljani
Copy link

ljani commented Jan 17, 2019

Documentation could use a little improvement: If either DoSomething or Setup fails, does Cleanup run? Also, when is the triggered Target ran, as early as possible or as late as possible?

Empirically:

  • Cleanup does not run if build fails (not so good for my particular case)
  • They're ran as late as possible (good!)

@matkoch
Copy link
Member

matkoch commented Jan 17, 2019

@ljani the example used in my comment here is not a good one, as it suggests that this works like IDisposable. However, it doesn't, and that was not the intent.

For your case to work, you would need to set a flag in the middle part, similar to this:

bool DoSomethingSucceeded = false;

Target Setup => _ => _
   .Executes(() => {});

Target DoSomething => _ => _
   .DependsOn(Setup)
   .Triggers(Cleanup)
   .Executes(() =>
   {
       try { }
       catch { DoSomethingSucceeded = true; }
   });

Target Cleanup => _ => _
   .OnlyWhenDynamic(() => DoSomethingSucceeded)
   .Executes(() => {});

@matkoch
Copy link
Member

matkoch commented Jan 17, 2019

They're ran as late as possible (good!)

This is not intentionally either. You should use Before and After to declare ordering dependencies.

@matkoch
Copy link
Member

matkoch commented Jan 22, 2019

Getting back to this: do you think that a ContinueOnFailure would be of benefit for your use case? We would still mark the target as failed, but proceed executing all other targets of the build plan.

@ljani
Copy link

ljani commented Jan 22, 2019

Thanks for taking interest. I'm still collecting IDisposables during build and disposing them all at NukeBuild.OnBuildFinished. I'd say something similar to condition: always() would be ideal for me. In other words, that build step would be ran always, even if the previous ones failed, if it's on the final build pipeline.

^C is a little interesting special case for this. Currently I'm calling the same cleanup action as in OnBuildFinished when Console.CancelKeyPress triggers. If you wanted to handle that with condition: always(), I think you'd need to start handling ^C?

@matkoch
Copy link
Member

matkoch commented Jan 29, 2019

ContinueOnFailure and AssuredAfterFailure have been added in prerelease. You still need triggers or dependencies to make them execute.

^C is not planned in the near future, but you're welcome to create an issue for that.

@lock
Copy link

lock bot commented Nov 20, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants