Repository: glotzerlab/signac
Pull request #270 (2020-01-14 22:20:22)

Fix/tarfile import long sp
<!-- Provide a general summary of your changes in the Title above -->

## Description
As detailed [here](https://bugs.python.org/issue21987), Python tarfiles have inconsistent behavior with respect to trailing backslashes depending on the length of the path. As a result, exporting a signac data space to a tarball and reimporting that data space fails (it simply ignores certain jobs) if the paths are too long because the schema function fails. The issue was introduced by #266 because we are no longer using `os.path.join`, which introduces backward slashes on Windows for tarballs even though those paths actually use forward slashes, but did correctly handle the trailing backslash.

## Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
<!-- If it fixes an open issue, please link to the issue here. -->
This issue was picked up in development of signac-flow, where the issue arises because the template testing introduces some very long names. It needs to be fixed because tests on signac-flow are using signac master.

## Types of Changes
<!-- Please select all items that apply either now or after creating the pull request: -->
- [ ] Documentation update
- [x] Bug fix
- [ ] New feature
- [ ] Breaking change<sup>1</sup>

<sup>1</sup>The change breaks (or has the potential to break) existing functionality.

## Checklist:
<!-- Please select all items that apply either now or after creating the pull request. -->
<!-- If you are unsure about any of these items, do not hesitate to ask! -->
- [x] I am familiar with the [**Contributing Guidelines**](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [x] I agree with the terms of the [**Contributor Agreement**](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [x] My code follows the [code style guideline](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md#code-style) of this project.
- [x] The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:
- [ ] I have updated the API documentation as part of the package doc-strings.
- [ ] I have created a separate pull request to update the [framework documentation](https://docs.signac.io/) on [signac-docs](https://github.com/glotzerlab/signac-docs) and linked it here.
- [ ] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt) and added all related issue and pull request numbers for future reference (if applicable). See example below.


Example for a changelog entry: `Fix issue with launching rockets to the moon (#101, #212).`


codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/270?src=pr&el=h1) Report
> Merging [#270](https://codecov.io/gh/glotzerlab/signac/pull/270?src=pr&el=desc) into [master](https://codecov.io/gh/glotzerlab/signac/commit/6bf33e097e6545fe7e1d14fddd6e2f35cd23a4bd?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/glotzerlab/signac/pull/270/graphs/tree.svg?width=650&token=FghbjTyzgM&height=150&src=pr)](https://codecov.io/gh/glotzerlab/signac/pull/270?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
- Coverage   65.01%   64.98%   -0.04%     
==========================================
  Files          40       40              
  Lines        5626     5629       +3     
==========================================
  Hits         3658     3658              
- Misses       1968     1971       +3
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/270?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/contrib/import\_export.py](https://codecov.io/gh/glotzerlab/signac/pull/270/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvaW1wb3J0X2V4cG9ydC5weQ==) | `84.89% <100%> (+0.11%)` | :arrow_up: |
| [signac/contrib/project.py](https://codecov.io/gh/glotzerlab/signac/pull/270/diff?src=pr&el=tree#diff-c2lnbmFjL2NvbnRyaWIvcHJvamVjdC5weQ==) | `90.43% <0%> (-0.44%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/270?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/270?src=pr&el=footer). Last update [6bf33e0...43dd172](https://codecov.io/gh/glotzerlab/signac/pull/270?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/signac/contrib/import_export.py b/signac/contrib/import_export.py
index a475e367..05c439ac 100644
--- a/signac/contrib/import_export.py
+++ b/signac/contrib/import_export.py
@@ -538,11 +538,23 @@ class _CopyFromTarFileExecutor(object):
         return _copy_to_job_workspace(self.src, self.job, shutil.copytree)
 
 
+def _tarfile_path_join(path, fn):
+    """Replacement for os.path.join that always uses forward slashes.
+
+    Due to this bug in Python tarfile https://bugs.python.org/issue21987 we may
+    or may not have a trailing backslash in the provided path. Rather than
+    checking the exact length, which could lead to backwards incompatibilities,
+    we simply strip trailing slashes and always add them back.
+    """
+    path = path.rstrip('/')
+    return path + '/' + fn
+
+
 def _analyze_tarfile_for_import(tarfile, project, schema, tmpdir):
 
     def read_sp_manifest_file(path):
         # Must use forward slashes, not os.path.sep.
-        fn_manifest = path + '/' + project.Job.FN_MANIFEST
+        fn_manifest = _tarfile_path_join(path, project.Job.FN_MANIFEST)
         try:
             with closing(tarfile.extractfile(fn_manifest)) as file:
                 if sys.version_info < (3, 6):


Test patch:
diff --git a/tests/test_project.py b/tests/test_project.py
index d1ee5840..32147f77 100644
--- a/tests/test_project.py
+++ b/tests/test_project.py
@@ -1092,6 +1092,20 @@ class ProjectExportImportTest(BaseProjectTest):
         self.assertEqual(len(self.project), 10)
         self.assertEqual(ids_before_export, list(sorted(self.project.find_job_ids())))
 
+    def test_export_import_tarfile_zipped_longname(self):
+        """Test the behavior of tarfile export when the path is >100 chars."""
+        target = os.path.join(self._tmp_dir.name, 'data.tar.gz')
+        val_length = 100
+        self.project.open_job(dict(a='1'*val_length)).init()
+        self.project.open_job(dict(a='2'*val_length)).init()
+        self.project.export_to(target=target)
+        # Jobs are always copied, not moved, when writing to a tarfile, so we
+        # must remove them manually to ensure that they're regenerated.
+        for job in self.project:
+            job.remove()
+        self.project.import_from(origin=target)
+        self.assertEqual(len(self.project), 2)
+
     def test_export_import_tarfile_zipped(self):
         target = os.path.join(self._tmp_dir.name, 'data.tar.gz')
         for i in range(10):