Warn strongly if the rsync file syncer is not in use

Just implemented status report entry:

The tests, they are a-failin'...

Unassigning since I haven't had time for this and won't soon.

Note that I believe that we could install rsync on DrupalCI. See https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes... → .

But we should also just request them to install it — seems like that'd not be a big deal. Tagging for that — the follow-up is necessary not here but in the DrupalCI issue queue!

@Wim Leers I assuming that #6 means we could install rsync in drupalci.yml

How hard would that be? Should we just try it here? If so we might want to add data provider to our build tests to run both copiers.

One concern I have is we might recommend something we don't test. We mention briefly in discussions that maybe that is ok because composer stage is testing rsync but there is no mention in the issue summary.

Turns out that we don't particularly encourage the use of rsync, which does perform integrity checks.

So rsync does integrity checks after or during the copy?

If rsync fails in integrity checks are we thinking that logic is \Drupal\package_manager\StageBase::apply would catch this and throw a ApplyFailedException error where the php syncer would just not check?

So rsync does integrity checks after or during the copy?

Quoting man rsync:

       -c, --checksum
              This forces the sender to checksum every regular file using a 128-bit MD4 checksum.  It does this during the initial file-
              system scan as it builds the list of all available files. The receiver then checksums its version of each file (if it exists
              and it has the same size as its sender-side counterpart) in order to decide which files need to be updated: files with either
              a changed size or a changed checksum are selected for transfer.  Since this whole-file checksumming of all files on both sides
              of the connection occurs in addition to the automatic checksum verifications that occur during a file's transfer, this option
              can be quite slow.

              Note that rsync always verifies that each transferred file was correctly reconstructed on the receiving side by checking its
              whole-file checksum, but that automatic after-the-transfer verification has nothing to do with this option's before-the-
              transfer "Does this file need to be updated?" check.

Note that \PhpTuf\ComposerStager\Infrastructure\Service\FileSyncer\RsyncFileSyncer does not set --checksum or -c. But it's that second paragraph that explains why that's simply unnecessary: rsync always checksums when transferring data.

If rsync fails in integrity checks are we thinking that logic is \Drupal\package_manager\StageBase::apply would catch this and throw a ApplyFailedException error where the php syncer would just not check?

Yes:

        try {
            $this->rsync->run($command, $callback);
        } catch (ExceptionInterface $e) {
            throw new IOException($e->getMessage(), 0, $e);
        }

would throw and

    try {
      $this->committer->commit($stage_dir, $active_dir, $paths_to_exclude, NULL, $timeout);
    }
    catch (InvalidArgumentException | PreconditionException $e) {
…
    }
    catch (\Throwable $throwable) {
      // The commit operation may have failed midway through, and the site code
      // is in an indeterminate state. Release the flag which says we're still
      // applying, because in this situation, the site owner should probably
      // restore everything from a backup.
      $this->setNotApplying()();
      throw new ApplyFailedException($this, (string) $this->getFailureMarkerMessage(), $throwable->getCode(), $throwable);
    }

would catch it and would contain the stderr output (starting with \Symfony\Component\Process\Exception\ProcessFailedException).

#6:

Note that I believe that we could install rsync on DrupalCI. See https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes → ....

Done: 📌 Use the rsync file syncer by default Fixed .

We don't need postpone this issue because I think there are still actionable steps here but we should not commit it until we make sure the test failures are rsync not that we actually testing it in 📌 Use the rsync file syncer by default Fixed are resolved.

creating a validator

Discussed with @omkar.podey:

1. First, install rsync on DrupalCI (commit #3362143-10: Install rsync on DrupalCI and run build tests with it → as a commit in this MR).
2. This should make tests pass. Or should allow you to make them pass! 😄
3. Once tests are green, implement the final step: the second step of the issue summary's proposed resolution, since so far it only implements the first step.

Because the proposed resolution in the issue summary does already say this:

1. Status report entry that strongly recommends the use of rsync and warns the user if that's not being used.
2. Detect presence of rsync during installation and then automatically switch to rsync instead of php file syncer (which is the sensible default: it always works).

@tedbow tagged this in #7. But that was specifically for ensuring that our recommendation actually works: i.e. that tests can pass when using rsync.

That is exactly what 📌 Use the rsync file syncer by default Fixed is for (which was opened after #7). So marking this postponed on that issue, but we can already get this issue 100% ready, so it can land immediately after we sort out the remaining problems in #3362143 👍

The failure occurs in build test

Could not delete /private/tmp/.package_managera198d4e2-1eeb-4d1b-9ce4-48c92dde9305/DrY-pNX7W3z6XKjoN3EUJ1Zo-R1WV5zk27haBgWLAMc/web/sites/default/default.services.yml: 
Could not delete /private/tmp/.package_managera198d4e2-1eeb-4d1b-9ce4-48c92dde9305/DrY-pNX7W3z6XKjoN3EUJ1Zo-R1WV5zk27haBgWLAMc/web/sites/default/default.settings.php: 

Tests failing. I haven't looked further yet

Blocker is in.

Needs work, see MR comment

I think this looks good but could use other opinions on the wording.

I helped @omkar.podey:

• to figure out a way forward here, because it is physically impossible for a validator to show a validation result that depends on the TERMINATE event (because it happens after the status report page has rendered!)
• recover the functional test coverage from 📌 Module does not work with Automated Cron Closed: won't fix

the comment just above is for another issue, i have ported the comment to 📌 Disallow unattended updates if performed by automated_cron Fixed

changing the title because there are know problem with the PHP syncer.

See MR comments

I think this looks good but tests failed

Looks good. I just removed one left over change

Looks superb now!

Just one question. One line in hook_help() that I think is obsolete.

Why was this committed without actually answering my question? I don't understand why a "I'm not sure" is followed by a commit?

I'm not sure either, to be honest, but we do account for the possibility (during status check) that they might be using the PHP file syncer, somehow. If they are, it seemed unfair to just say "don't use that!" without explaining how to change it back.

So I still think that this is essentially noise in hook_help():
$output .= '<p>' . t('If <code>rsync is available but Package Manager is configured to use the built-in PHP file syncer, you should also add the following to settings.php:') . '

';
$output .= "

\$config['package_manager.settings']['file_syncer'] = 'rsync';

";

But … since this is "just" hook_help() which will change anyway when going into core … not reopening this.

Automatically closed - issue fixed for 2 weeks with no activity.