Wrong dependency order after updating to the latest Fake causes tests to be run before built #1515
Comments
|
@BlythMeister can you please take a quick look at this? I assume it's related to your recent changes. |
|
Yeah sure, can you please provide your dependency trees from the fsx file. Just looking at the cut down tree, it looks like your tests are not dependent on the build. If this is the case, it previously worked a bit by luck. |
|
Should be at the bottom of https://github.com/fsharp/FSharp.Data/blob/master/build.fsx |
|
Yeah just realised that. If you have a dependency between targets you should state that in the dependencies for the build. |
|
I think the problem is at https://github.com/fsharp/FSharp.Data/blob/master/build.fsx#L126 @ovatsus you define that the targets should run before the runtest target. That condition is fulfilled. I think want you want is to reverse the arrow. |
|
@BlythMeister yes it seems to be unspecified in the script. I will send a pull request to fsharp.data. But it's still a bit unfortunate that we changed behavior here. |
|
Agreed, it's a shame the behaviour changed. But arguably it's now correct in that it obeys the dependencies you set in your script rather than being down to chance/ordering. If you don't state that b is dependent on a, then that states they can run in any order. I had assumed in making this change that dependencies were specified by users. Maybe this was a bad assumption. |
|
It obeyed it before as well. |
|
True, but a lot of it came down to the order dependency was specified rather than making targets actually dependant. I can try and make those orders be the same without having to make the change? |
|
I just looked at @ovatsus's project |
|
@BlythMeister I think we should restore that behaviour |
|
Ok, I'll have a look into why that situation does not work. |
|
thanks! |
|
This is the commit that broke it i think... e87f690 I didn't know that target specification ordering impacted target ordering. |
|
@BlythMeister it was never intended to do so, but since important project rely on it... |
|
Ahh as i expected...it worked by chance 😋 I will try and restore it. |
|
@forki I've had a look at this, and it's because it's running the targets as early as possible now. Since those "RunTests_" are not dependant on anything, they can run really early. Previously targets ran in the order they were visited, hence why they would run later in the build. The way the current solution works does take into account the ordering in the file to state the order of execution, but "RunTests_" is defined ahead of all the other targets. The way i see it we have 2 choices.
|
|
I'm tending towards 2 |
|
The reason I did not specify test to depend on build, was that I wanted to be able to do 'build RunTests' without forcing to build again, that's very useful when manually editing some tests and just reruning. There are probably a lot of projects with similar issues as the FSharp.Data build script was then used as an example for Deedle and the for project scaffolding, so it would be great if we could get the old behaviour. Or if it's still possible to achieve what I had in a different way, I'm happy to change the scrip as well |
|
I feared you would say that...it feels wrong to me. Things should run as early as they can. Maybe we could have a flag set to retain old run function? A "RunInVisitOrder" environment variable? |
|
Things should run as early as they can.
why? that's not really needed
2017-04-11 10:06 GMT+02:00 Chris Blyth <notifications@github.com>:
… I feared you would say that...it feels wrong to me.
Things should run as early as they can.
Maybe we could have a flag set to retain old run function?
A "RunInVisitOrder" environment variable?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1515 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AADgNFgmwjwwTJR07wGbPLTlEfsnNuKrks5ruzRwgaJpZM4M5avM>
.
|
|
@ovatsus there is a run-single-target command line parameter for FAKE. But you are probably not the only one |
|
@forki running dependencies early means it fails faster. I am happy to revert the logic if you think that is best. |
|
yes we see that here. it fails pretty fast ;-) |
|
haha :) @ovatsus i don't understand the scenario where you would want to run something you have yet to build |
|
@BlythMeister it's already built. he just wants to rerun tests |
|
@forki would you accept a configuration switch to use old logic? that way, if someone has issues, they can revert to the old logic? |
|
tbh. I don't see why that's needed. We usually strive to be backwards compatible and don't want to break existing build scripts. usualy these scripts are not touched for very long time. |
|
ok, i'll get a revert in so single thread just runs in the order they are visited rather than as early as possible. |
|
yes that sounds good. |
|
revert raised, should probably be tested on your PR to f# data? |
|
i guess on a parallel build, it makes sense to run things as early as possible, but on a non parallel, it does not. I think this code could be simplified to reflect that... |
|
@forki suggest this should be closed now? |
Description
Before:
After
Repro steps
Run
build Allon the current master of FSharp.Data - fsprojects/FSharp.Data@4012bd9 - everything works as expected (first screenshot)Run
paket update- ovatsus/FSharp.Data@2571a15 - wrong dependency order (second screenshot)The text was updated successfully, but these errors were encountered: