rimport now does relinking step.

Resolves #14.

Author: samsrabin

relink.py

@@ -222,6 +222,7 @@ def replace_files_with_symlinks(
     for file_path in find_owned_files_scandir(
         item_to_process, user_uid, inputdata_root
     ):
+        logger.info("'%s':", file_path)
         replace_one_file_with_symlink(
             inputdata_root, target_dir, file_path, dry_run=dry_run
         )
@@ -238,7 +239,6 @@ def replace_one_file_with_symlink(inputdata_root, target_dir, file_path, dry_run
         file_path (str): The path of the file to be replaced.
         dry_run (bool): If True, only show what would be done without making changes.
     """
-    logger.info("'%s':", file_path)
 
     # Determine the relative path and the new link's destination
     relative_path = os.path.relpath(file_path, inputdata_root)
@@ -270,7 +270,7 @@ def replace_one_file_with_symlink(inputdata_root, target_dir, file_path, dry_run
         os.rename(link_name, link_name + ".tmp")
         logger.info("%sDeleted original file: %s", INDENT, link_name)
     except OSError as e:
-        logger.error("%sError deleting file %s: %s. Skipping.", INDENT, link_name, e)
+        logger.error("%sError deleting file %s: %s. Skipping relink.", INDENT, link_name, e)
         return
 
     # Create the symbolic link, handling necessary parent directories
@@ -283,7 +283,7 @@ def replace_one_file_with_symlink(inputdata_root, target_dir, file_path, dry_run
     except OSError as e:
         os.rename(link_name + ".tmp", link_name)
         logger.error(
-            "%sError creating symlink for %s: %s. Skipping.", INDENT, link_name, e
+            "%sError creating symlink for %s: %s. Skipping relink.", INDENT, link_name, e
         )
 
 

rimport

@@ -18,6 +18,8 @@ from typing import Iterable, List
 from urllib.request import Request, urlopen
 from urllib.error import HTTPError
 
+from relink import replace_one_file_with_symlink
+
 import shared
 INDENT = shared.INDENT
 
@@ -143,10 +145,24 @@ def normalize_paths(root: Path, relnames: Iterable[str]) -> List[Path]:
     return paths
 
 
+def check_relink_worked(src: Path, dst: Path) -> None:
+    """Check whether relink worked
+
+    Args:
+        src (Path): Source file (should have been converted to symlink)
+        dst (Path): Destination file (symlink target)
+
+    Raises:
+        RuntimeError: If src is not a symlink pointing to dst.
+    """
+    if not (src.is_symlink() and src.resolve() == dst):
+        raise RuntimeError("Error relinking during rimport")
+
+
 def stage_data(
     src: Path, inputdata_root: Path, staging_root: Path, check: bool = False
 ) -> None:
-    """Stage a file by mirroring its path under `staging_root`.
+    """Stage a file by mirroring its path under `staging_root`, then replace with symlink to staged.
 
     Destination path is computed by replacing the `inputdata_root` prefix of `src`
     with `staging_root`, i.e.:
@@ -162,6 +178,7 @@ def stage_data(
         RuntimeError: If `src` is a live symlink pointing outside staging, or if `src` is outside
                       the inputdata root, or if `src` is already under staging directory.
         RuntimeError: If `src` is a broken symlink.
+        RuntimeError: If it failed to replace `src` with a symlink to the staged file.
         FileNotFoundError: If `src` does not exist.
 
     Guardrails:
@@ -199,20 +216,25 @@ def stage_data(
     dst = staging_root / rel
 
     if dst.exists():
-        logger.info("%sFile is already published but NOT linked; do", INDENT)
-        logger.info("%srelink.py %s", 2 * INDENT, rel)
-        logger.info("%sto resolve.", INDENT)
+        logger.info("File is already published but NOT linked; linking now.")
+        replace_one_file_with_symlink(inputdata_root, staging_root, str(src))
         print_can_file_be_downloaded(can_file_be_downloaded(rel, staging_root))
+        check_relink_worked(src, dst)
         return
 
     if check:
         logger.info("%sFile is not already published", INDENT)
         return
 
+    # Copy file to destination
     dst.parent.mkdir(parents=True, exist_ok=True)
     shutil.copy2(src, dst)
     logger.info("%s[rimport] staged %s -> %s", INDENT, src, dst)
 
+    # Replace original with symlink to destination
+    replace_one_file_with_symlink(inputdata_root, staging_root, str(src))
+    check_relink_worked(src, dst)
+
 
 def ensure_running_as(target_user: str, argv: list[str]) -> None:
     """Ensure the script is running as the target user, re-executing via sudo if needed.

tests/relink/test_replace_files_with_symlinks.py

@@ -12,6 +12,8 @@
 from unittest.mock import patch, call
 import pytest
 
+import shared
+
 # Add parent directory to path to import relink module
 sys.path.insert(
     0, os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
@@ -20,14 +22,23 @@
 import relink  # noqa: E402
 
 
+@pytest.fixture(autouse=True)
+def configure_logging_for_tests():
+    """Configure logging for all tests in this module."""
+    shared.configure_logging(logging.INFO)
+    yield
+    # Cleanup
+    relink.logger.handlers.clear()
+
+
 @pytest.fixture(name="mock_replace_one")
 def fixture_mock_replace_one():
     """Fixture that mocks relink.replace_one_file_with_symlink"""
     with patch("relink.replace_one_file_with_symlink") as mock:
         yield mock
 
 
-def test_basic_file_replacement_given_dir(temp_dirs, current_user, mock_replace_one):
+def test_basic_file_replacement_given_dir(temp_dirs, current_user, mock_replace_one, caplog):
     """Test basic functionality: given directory, replace owned file with symlink."""
     inputdata_root, target_dir = temp_dirs
     username = current_user
@@ -55,8 +66,11 @@ def test_basic_file_replacement_given_dir(temp_dirs, current_user, mock_replace_
         dry_run=False,
     )
 
+    # Verify message with filename was printed
+    assert f"'{source_file}':" in caplog.text
 
-def test_basic_file_replacement_given_file(temp_dirs, current_user, mock_replace_one):
+
+def test_basic_file_replacement_given_file(temp_dirs, current_user, mock_replace_one, caplog):
     """Test basic functionality: given owned file, replace with symlink."""
     inputdata_root, target_dir = temp_dirs
     username = current_user
@@ -84,8 +98,11 @@ def test_basic_file_replacement_given_file(temp_dirs, current_user, mock_replace
         dry_run=False,
     )
 
+    # Verify message with filename was printed
+    assert f"'{source_file}':" in caplog.text
+
 
-def test_dry_run(temp_dirs, current_user, mock_replace_one):
+def test_dry_run(temp_dirs, current_user, mock_replace_one, caplog):
     """Test that dry_run=True is passed correctly."""
     inputdata_root, target_dir = temp_dirs
     username = current_user
@@ -117,6 +134,9 @@ def test_dry_run(temp_dirs, current_user, mock_replace_one):
         dry_run=True,
     )
 
+    # Verify message with filename was printed
+    assert f"'{source_file}':" in caplog.text
+
 
 def test_nested_directory_structure(temp_dirs, current_user, mock_replace_one):
     """Test with nested directory structures."""
@@ -175,6 +195,9 @@ def test_skip_existing_symlinks(temp_dirs, current_user, caplog, mock_replace_on
     # Verify replace_one_file_with_symlink() wasn't called
     mock_replace_one.assert_not_called()
 
+    # Verify message with filename was NOT printed
+    assert f"'{source_link}':" not in caplog.text
+
 
 def test_missing_target_file(temp_dirs, current_user, caplog, mock_replace_one):
     """Test behavior when target file doesn't exist."""
@@ -224,7 +247,7 @@ def test_invalid_username(temp_dirs, caplog, mock_replace_one):
     mock_replace_one.assert_not_called()
 
 
-def test_multiple_files(temp_dirs, current_user, mock_replace_one):
+def test_multiple_files(temp_dirs, current_user, mock_replace_one, caplog):
     """Test with multiple files in the directory."""
     inputdata_root, target_dir = temp_dirs
     username = current_user
@@ -251,6 +274,11 @@ def test_multiple_files(temp_dirs, current_user, mock_replace_one):
         calls.append(call(inputdata_root, target_dir, source_file, dry_run=False))
     mock_replace_one.assert_has_calls(calls, any_order=True)
 
+    # Verify message with filename was printed
+    for i in range(5):
+        source_file = os.path.join(inputdata_root, f"file_{i}.txt")
+        assert f"'{source_file}':" in caplog.text
+
 
 def test_multiple_files_nested(temp_dirs, current_user, mock_replace_one):
     """Test with multiple files scattered throughout a nested directory tree."""

tests/relink/test_replace_one_file_with_symlink.py

@@ -118,7 +118,7 @@ def test_absolute_paths(temp_dirs):
         os.chdir(cwd)
 
 
-def test_print_found_owned_file(temp_dirs, caplog):
+def test_no_print_found_owned_file(temp_dirs, caplog):
     """Test that message with filename is printed."""
     source_dir, target_dir = temp_dirs
 
@@ -135,9 +135,8 @@ def test_print_found_owned_file(temp_dirs, caplog):
     with caplog.at_level(logging.INFO):
         relink.replace_one_file_with_symlink(source_dir, target_dir, source_file)
 
-    # Check that message was logged
-    assert f"'{source_file}':" in caplog.text
-    assert source_file in caplog.text
+    # Check that message was NOT logged (should happen in replace_files_with_symlinks instead)
+    assert f"'{source_file}':" not in caplog.text
 
 
 def test_print_deleted_and_created_messages(temp_dirs, caplog):

tests/rimport/test_check_relink_worked.py

@@ -0,0 +1,61 @@
+"""
+Tests for check_relink_worked() function in rimport script.
+"""
+
+import os
+import importlib.util
+from importlib.machinery import SourceFileLoader
+
+import pytest
+
+
+# Import rimport module from file without .py extension
+rimport_path = os.path.join(
+    os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))),
+    "rimport",
+)
+loader = SourceFileLoader("rimport", rimport_path)
+spec = importlib.util.spec_from_loader("rimport", loader)
+if spec is None:
+    raise ImportError(f"Could not create spec for rimport from {rimport_path}")
+rimport = importlib.util.module_from_spec(spec)
+# Don't add to sys.modules to avoid conflict with other test files
+loader.exec_module(rimport)
+
+def test_ok(tmp_path):
+    """Check that it doesn't error if src is a symlink pointing to dst"""
+    # Set up
+    dst = tmp_path / "dst.nc"
+    src = tmp_path / "src.nc"
+    src.symlink_to(dst)
+
+    # Shouldn't error
+    rimport.check_relink_worked(src, dst)
+
+def test_error_not_symlink(tmp_path):
+    """Check that it does error if src isn't a symlink"""
+    # Set up
+    dst = tmp_path / "dst.nc"
+    src = tmp_path / "src.nc"
+
+    # Should error
+    with pytest.raises(RuntimeError) as exc_info:
+        rimport.check_relink_worked(src, dst)
+
+    # Verify error message was printed
+    assert "Error relinking during rimport" in str(exc_info.value)
+
+def test_error_symlink_but_not_to_dst(tmp_path):
+    """Check that it does error if src is a symlink but not pointing to dst"""
+    # Set up
+    dst = tmp_path / "dst.nc"
+    other = tmp_path / "other.nc"
+    src = tmp_path / "src.nc"
+    src.symlink_to(other)
+
+    # Should error
+    with pytest.raises(RuntimeError) as exc_info:
+        rimport.check_relink_worked(src, dst)
+
+    # Verify error message was printed
+    assert "Error relinking during rimport" in str(exc_info.value)

tests/rimport/test_cmdline.py

@@ -82,6 +82,10 @@ def test_file_option_stages_single_file(
         assert staged_file.exists()
         assert staged_file.read_text() == "test data"
 
+        # Verify file was relinked
+        assert test_file.is_symlink()
+        assert test_file.resolve() == staged_file
+
     def test_list_option_stages_multiple_files(
         self, rimport_script, test_env, rimport_env
     ):
@@ -127,6 +131,12 @@ def test_list_option_stages_multiple_files(
         assert (staging_root / "file1.nc").read_text() == "data1"
         assert (staging_root / "file2.nc").read_text() == "data2"
 
+        # Verify both files were relinked
+        assert file1.is_symlink()
+        assert file1.resolve() == (staging_root / "file1.nc")
+        assert file2.is_symlink()
+        assert file2.resolve() == (staging_root / "file2.nc")
+
     def test_preserves_directory_structure(self, rimport_script, test_env, rimport_env):
         """Test that directory structure is preserved in staging."""
         inputdata_root = test_env["inputdata_root"]
@@ -163,6 +173,10 @@ def test_preserves_directory_structure(self, rimport_script, test_env, rimport_e
         assert staged_file.exists()
         assert staged_file.read_text() == "nested data"
 
+        # Verify file was relinked
+        assert nested_file.is_symlink()
+        assert nested_file.resolve() == staged_file
+
     def test_error_for_nonexistent_file(self, rimport_script, test_env, rimport_env):
         """Test that error is reported for nonexistent file."""
         inputdata_root = test_env["inputdata_root"]
@@ -308,6 +322,12 @@ def test_list_with_comments_and_blanks(self, rimport_script, test_env, rimport_e
         assert (staging_root / "file1.nc").exists()
         assert (staging_root / "file2.nc").exists()
 
+        # Verify both files were relinked
+        assert file1.is_symlink()
+        assert file1.resolve() == (staging_root / "file1.nc")
+        assert file2.is_symlink()
+        assert file2.resolve() == (staging_root / "file2.nc")
+
     def test_prints_and_exits_for_already_published_linked_file(
         self, rimport_script, test_env, rimport_env
     ):
@@ -473,5 +493,8 @@ def test_check_doesnt_copy(self, rimport_script, test_env, rimport_env):
         staged_file = staging_root / "test.nc"
         assert not staged_file.exists()
 
+        # Verify file was not replaced with a symlink
+        assert not test_file.is_symlink()
+
         # Verify message was printed
         assert "not already published" in result.stdout