From b1d5fcef90adc11ad4db0d35b9b8c0a881650ca4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 20 Jul 2024 14:52:00 +0200 Subject: [PATCH 1/3] Fix warnings when files are overwritten in the build directory. --- CHANGES.rst | 2 +- sphinx/util/fileutil.py | 4 ++-- sphinx/util/osutil.py | 11 ++++++++--- tests/roots/test-util-copyasset_overwrite/myext.py | 4 ++-- tests/test_util/test_util_fileutil.py | 1 - 5 files changed, 13 insertions(+), 9 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 66150d2624a..1686f2cbc8a 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -5,7 +5,7 @@ Bugs fixed ---------- * #12096: Warn when files are overwritten in the build directory. - Patch by Adam Turner. + Patch by Adam Turner and Bénédikt Tran. * #12620: Ensure that old-style object description options are respected. Patch by Adam Turner. * #12601, #12625: Support callable objects in :py:class:`~typing.Annotated` type diff --git a/sphinx/util/fileutil.py b/sphinx/util/fileutil.py index b79fd0fc5ca..de1ddce2caa 100644 --- a/sphinx/util/fileutil.py +++ b/sphinx/util/fileutil.py @@ -75,8 +75,8 @@ def copy_asset_file(source: str | os.PathLike[str], destination: str | os.PathLi msg = ('Copying the rendered template %s to %s will overwrite data, ' 'as a file already exists at the destination path ' 'and the content does not match.') - logger.info(msg, os.fsdecode(source), os.fsdecode(destination), - type='misc', subtype='copy_overwrite') + logger.warning(msg, os.fsdecode(source), os.fsdecode(destination), + type='misc', subtype='copy_overwrite') destination = _template_basename(destination) or destination with open(destination, 'w', encoding='utf-8') as fdst: diff --git a/sphinx/util/osutil.py b/sphinx/util/osutil.py index 474fc5a2bb7..d738f3f9642 100644 --- a/sphinx/util/osutil.py +++ b/sphinx/util/osutil.py @@ -106,7 +106,12 @@ def copyfile( msg = f'{os.fsdecode(source)} does not exist' raise FileNotFoundError(msg) - if not (dest_exists := path.exists(dest)) or not filecmp.cmp(source, dest): + if ( + not (dest_exists := path.exists(dest)) or + # comparison must be done using shallow=False since + # two different files might have the same bytes size + not filecmp.cmp(source, dest, shallow=False) + ): if __overwrite_warning__ and dest_exists: # sphinx.util.logging imports sphinx.util.osutil, # so use a local import to avoid circular imports @@ -116,8 +121,8 @@ def copyfile( msg = ('Copying the source path %s to %s will overwrite data, ' 'as a file already exists at the destination path ' 'and the content does not match.') - logger.info(msg, os.fsdecode(source), os.fsdecode(dest), - type='misc', subtype='copy_overwrite') + logger.warning(msg, os.fsdecode(source), os.fsdecode(dest), + type='misc', subtype='copy_overwrite') shutil.copyfile(source, dest) with contextlib.suppress(OSError): diff --git a/tests/roots/test-util-copyasset_overwrite/myext.py b/tests/roots/test-util-copyasset_overwrite/myext.py index b4fd59749fb..544057c1fc3 100644 --- a/tests/roots/test-util-copyasset_overwrite/myext.py +++ b/tests/roots/test-util-copyasset_overwrite/myext.py @@ -6,14 +6,14 @@ def _copy_asset_overwrite_hook(app): css = app.outdir / '_static' / 'custom-styles.css' # html_static_path is copied by default - assert css.read_text() == '/* html_static_path */\n' + assert css.read_text() == '/* html_static_path */\n', 'invalid default text' # warning generated by here copy_asset( Path(__file__).parent.joinpath('myext_static', 'custom-styles.css'), app.outdir / '_static', ) # This demonstrates the overwriting - assert css.read_text() == '/* extension styles */\n' + assert css.read_text() == '/* extension styles */\n', 'overwriting failed' return [] diff --git a/tests/test_util/test_util_fileutil.py b/tests/test_util/test_util_fileutil.py index 4603257034a..fa890f7188b 100644 --- a/tests/test_util/test_util_fileutil.py +++ b/tests/test_util/test_util_fileutil.py @@ -106,7 +106,6 @@ def excluded(path): assert not (destdir / '_templates' / 'sidebar.html').exists() -@pytest.mark.xfail(reason='Filesystem chicanery(?)') @pytest.mark.sphinx('html', testroot='util-copyasset_overwrite') def test_copy_asset_overwrite(app): app.build() From d2e8003108453879d191d0f80b90f35214793fa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 20 Jul 2024 15:00:59 +0200 Subject: [PATCH 2/3] address review --- sphinx/util/fileutil.py | 6 ++++-- sphinx/util/osutil.py | 6 +++--- tests/test_util/test_util_fileutil.py | 7 +++---- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/sphinx/util/fileutil.py b/sphinx/util/fileutil.py index de1ddce2caa..5ba2c17b196 100644 --- a/sphinx/util/fileutil.py +++ b/sphinx/util/fileutil.py @@ -75,8 +75,10 @@ def copy_asset_file(source: str | os.PathLike[str], destination: str | os.PathLi msg = ('Copying the rendered template %s to %s will overwrite data, ' 'as a file already exists at the destination path ' 'and the content does not match.') - logger.warning(msg, os.fsdecode(source), os.fsdecode(destination), - type='misc', subtype='copy_overwrite') + # See https://github.com/sphinx-doc/sphinx/pull/12627#issuecomment-2241144330 + # for the rational for logger.info(). + logger.info(msg, os.fsdecode(source), os.fsdecode(destination), + type='misc', subtype='copy_overwrite') destination = _template_basename(destination) or destination with open(destination, 'w', encoding='utf-8') as fdst: diff --git a/sphinx/util/osutil.py b/sphinx/util/osutil.py index d738f3f9642..c5a856b22c7 100644 --- a/sphinx/util/osutil.py +++ b/sphinx/util/osutil.py @@ -109,7 +109,7 @@ def copyfile( if ( not (dest_exists := path.exists(dest)) or # comparison must be done using shallow=False since - # two different files might have the same bytes size + # two different files might have the same size not filecmp.cmp(source, dest, shallow=False) ): if __overwrite_warning__ and dest_exists: @@ -121,8 +121,8 @@ def copyfile( msg = ('Copying the source path %s to %s will overwrite data, ' 'as a file already exists at the destination path ' 'and the content does not match.') - logger.warning(msg, os.fsdecode(source), os.fsdecode(dest), - type='misc', subtype='copy_overwrite') + logger.info(msg, os.fsdecode(source), os.fsdecode(dest), + type='misc', subtype='copy_overwrite') shutil.copyfile(source, dest) with contextlib.suppress(OSError): diff --git a/tests/test_util/test_util_fileutil.py b/tests/test_util/test_util_fileutil.py index fa890f7188b..2071fc3fade 100644 --- a/tests/test_util/test_util_fileutil.py +++ b/tests/test_util/test_util_fileutil.py @@ -109,14 +109,13 @@ def excluded(path): @pytest.mark.sphinx('html', testroot='util-copyasset_overwrite') def test_copy_asset_overwrite(app): app.build() - warnings = strip_colors(app.warning.getvalue()) src = app.srcdir / 'myext_static' / 'custom-styles.css' dst = app.outdir / '_static' / 'custom-styles.css' - assert warnings == ( - f'WARNING: Copying the source path {src} to {dst} will overwrite data, ' + assert ( + f'Copying the source path {src} to {dst} will overwrite data, ' 'as a file already exists at the destination path ' 'and the content does not match.\n' - ) + ) in strip_colors(app.status.getvalue()) def test_template_basename(): From 3e1475d22e7d762a75ca40475cb0dd6e8d04fc58 Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+AA-Turner@users.noreply.github.com> Date: Sat, 20 Jul 2024 14:07:48 +0100 Subject: [PATCH 3/3] SP&G --- sphinx/util/fileutil.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sphinx/util/fileutil.py b/sphinx/util/fileutil.py index 5ba2c17b196..10bfc3bd610 100644 --- a/sphinx/util/fileutil.py +++ b/sphinx/util/fileutil.py @@ -76,7 +76,7 @@ def copy_asset_file(source: str | os.PathLike[str], destination: str | os.PathLi 'as a file already exists at the destination path ' 'and the content does not match.') # See https://github.com/sphinx-doc/sphinx/pull/12627#issuecomment-2241144330 - # for the rational for logger.info(). + # for the rationale for logger.info(). logger.info(msg, os.fsdecode(source), os.fsdecode(destination), type='misc', subtype='copy_overwrite')