jolicode / castor

🦫 DX oriented task runner and command launcher built with PHP.
https://castor.jolicode.com
MIT License
391 stars 20 forks source link

`fingerprint` unexpected behavior when file has changed after the command run #446

Closed TheoD02 closed 1 month ago

TheoD02 commented 4 months ago

Hi

I am encountering a logical behavior with the current implementation of fingerprint. I would like to discuss the possibility to handle the case I will demonstrate below.

Here is my use case :

if (! fingerprint(callback: static fn () => composer()->install(), fingerprint: fgp()->composer(), force: $forceVendor || $force)) {
    io()->note('Composer dependencies are already installed.');
} else {
    io()->success('Composer dependencies installed');
}

Simply the code will install the composer dependencies (command is run with --quiet for better visiblity)

The output of what is currently done by the script

image

But in that case, the second run of castor install should not run composer installation. Because in reality the file hasn't changed between the first and second installation

That behavior is actually due to current implementation.

https://github.com/jolicode/castor/blob/642a6a902dfffa5e00c437240bf03d6ceef13686/src/functions.php#L587-L597

We pass the computed $fingerprint directly on fingerprint method.

The checked and saved fingerprint is the same from start to the end of execution. And it seem that composer update the file without changing anything in it, but the fingerprint change.

image

What i have in mind is something like this :

function fingerprint(callable $callback, callable $fingerprint, bool $force = false, bool $delayed = true): bool
{
    $hash = $fingerprint();
    if ($force || !fingerprint_exists($hash)) {
        $callback();
        fingerprint_save($delayed ? $fingerprint() : $hash);

        return true;
    }

    return false;
}
// Current definition of fingerprint
function composer(): string
{
    return hasher()
        ->writeWithFinder(finder()->files()->in(path('app'))->name(['composer.json', 'composer.lock', 'symfony.lock']))
        ->finish()
    ;
}

// With the new implementation purposed (BC but we can support the two definition until 1.0 ? string|callable ?) 
public function composer(): callable
{
    return function () {
        return hasher()
            ->writeWithFinder(finder()->files()->in(path('app'))->name(['composer.json', 'composer.lock', 'symfony.lock']))
            ->finish()
        ;
    };
}

WDYT about this approach, did you have another idea ? :smile:

Waiting for feedbacks

joelwurtz commented 4 months ago

I don't like having callback, and i think your behavior can be fixed by using the FileHashStrategy::Content strategy instead of the FileHashStrategy::MTimes one

Also the proposed behavior may induce some bugs, i.e.:

  1. I have a fingerprint on a file
  2. I Update the file
  3. I run a command that launch a very long task when this fingerprint has changed
  4. I update the file before the command finish
  5. I rerun the command after the first one has finished -> no update (but it should have been run)
TheoD02 commented 4 months ago

I don't like having callback, and i think your behavior can be fixed by using the FileHashStrategy::Content strategy instead of the FileHashStrategy::MTimes one

Also the proposed behavior may induce some bugs, i.e.:

  1. I have a fingerprint on a file
  2. I Update the file
  3. I run a command that launch a very long task when this fingerprint has changed
  4. I update the file before the command finish
  5. I rerun the command after the first one has finished -> no update (but it should have been run)

Hi, thank for the response

Yes, I had thought of using FileHashStrategy::Content.

But this one can also be misleading, let's admit it:

I'm on the main branch with a file containing ABC => this will play the command then save it as a fingerprint.

Let's say I need to change branch to feat/anything here the file has been updated with XYZ so the command will also run.

But when I return to main the command won't run because ABC content is still in the fingerprint cache too.


Yes I see the use case, which is why I had the idea of including a delayed parameter to indicate that this fingerprint run will be saved in a different way.

But I think the case of user modification during a long task should use flock to prevent modification of this file?

IDK which is the best solution, it all depends on the use case

joelwurtz commented 1 month ago

See #494 this should fix your issue

joelwurtz commented 1 month ago

TL;DR; we introduce an id parameter to fingerprint functions, so now you would do the following :

if (! fingerprint(callback: static fn () => composer()->install(), id: 'composer-deps', fingerprint: fgp()->composer(), force: $forceVendor || $force)) {
    io()->note('Composer dependencies are already installed.');
} else {
    io()->success('Composer dependencies installed');
}

The id will be the cache key, so new values will invalidate old ones and you should no longer have an issue for this

TheoD02 commented 1 month ago

Cool i will test it ! thank you 🫶