[feature] Verbose option for apply_conandata_patches()

[iskunk]

What is your suggestion?

I'd like to have this...

-def apply_conandata_patches(conanfile):
+def apply_conandata_patches(conanfile, verbose=False):

...which does this when verbose=True...

 ======== Installing packages ========
 zlib/1.2.13: Calling source() in /home/username/.conan2/p/zlibef641b716caf2/s
+zlib/1.2.13: Applying: patches/0001-Fix-cmake.patch
+zlib/1.2.13: Applying: patches/0002-gzguts-xcode12-compile-fix.patch
 
 -------- Installing package zlib/1.2.13 (1 of 1) --------
 zlib/1.2.13: Building from source

...so that information on which patches were applied is recorded in the build log.

I have this implemented and ready to go.

Have you read the CONTRIBUTING guide?

• I've read the CONTRIBUTING guide

[memsharded]

Hi @iskunk

Actually I think that listing by default the patches being applied should be the default behavior. This is an important information that should be in the logs by default, not as an opt-in, don't you think? Would you like to contribute that PR? Thanks for your offer to contribute this.

[iskunk]

Sounds good. The verbose=False was to retain the current behavior as the default, but it's a different story if the default should change. That makes the parameter unnecessary (no point in declining this particular bit of verbosity), and eliminates the need for a doc push.

I'll update my code, and get a PR in.

[iskunk]

In the course of testing, I noticed that the patch() function already has some code to indicate when a patch is applied; it just wasn't getting used in the common case of a patch_file without a patch_description.

So I updated my change to work with that. The patch_description string is favored over patch_file, and patch_type is inferred to be either file or string (but this was also working as an undocumented parameter, for better or for worse).

Let me know if you want things done any differently here.

[memsharded]

I am having a look to the PR, and it doesn't sound bad, but the fact that it seems the tests were broken could mean that it is slightly a bit more complicated than it seems.
Maybe a simpler change like:

if patch_type or patch_description:
...
elif patch_file: # New addition, if no explicit printing we print our new stuff
      conanfile.output.info(f"Applying patch {patch_file}")

could make it?

[iskunk]

If existing tests are checking the output produced by patch(), and we're changing the output, then aren't those tests going to need updating anyway? Even if we do things differently only when neither patch_type nor patch_description are given, aren't there other tests that run patch() in that manner?

[memsharded]

If existing tests are checking the output produced by patch(), and we're changing the output, then aren't those tests going to need updating anyway?

The problem seems to be that some tests are checking that the output contains some "Applying (backport) patch", and instead getting:

++ mocked/ref: Apply patch (file): patches/0001-buildflatbuffers-cmake.patch
-- mocked/ref: Apply patch (backport): Needed to build with modern clang compilers.

So the changes are "destroying" a previously valid and useful message, that we don't want to lose. It is fine to add a new message, and that will not break the tests, but not replace the good message for a new one losing information.

[iskunk]

The problem seems to be that some tests are checking that the output contains some "Applying (backport) patch", and instead getting:

The assertions appear to be checking x == y instead of x in y. It seems that a new Apply patch (file) line is being added, rather than the Apply patch (backport) line getting lost.

There is a related issue that occurs when patch_file and patch_description are specified: Which of those two values should be given after the colon? I think the filename is more unambiguous/informative, but the patch() code already favored the description.

[memsharded]

The assertions appear to be checking x == y instead of x in y. It seems that a new Apply patch (file) line is being added, rather than the Apply patch (backport) line getting lost.

Oh, then that is good. Feel free to modify the test to check for in, and then check also the new line with in. I think there is value in having both lines, thanks!

[iskunk]

All right, I've rolled in an update to the test, and it now passes cleanly. Please double-check my changes.

[memsharded]

Closed by #14177 for 2.0.8