improve IOUtils.createTempFile and UserException.BadTmpDir (#4711)

* improving error message in UserException.BadTmpDir
  - updating the commit message to recommend --tmp-dir
  - propagating the causal exception when relevant
  - fixes #4709

* simplify and rename tempfile -> createTempFile
  - use Files.createTempFile instead of existing code using File
  - schedule temp directories for automatic deletion

* removing IOUtils.absolute methods and replacing with calls to Path.normalize

src/main/java/org/broadinstitute/hellbender/exceptions/UserException.java

@@ -2,16 +2,15 @@
 
 import htsjdk.samtools.SAMSequenceDictionary;
 import htsjdk.tribble.Feature;
+import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
 import org.broadinstitute.hellbender.tools.walkers.variantutils.ValidateVariants;
-import org.broadinstitute.hellbender.utils.SimpleInterval;
 import org.broadinstitute.hellbender.utils.help.HelpConstants;
 import org.broadinstitute.hellbender.utils.read.GATKRead;
 import org.broadinstitute.hellbender.utils.read.ReadUtils;
 
 import java.io.File;
 import java.nio.file.Path;
 import java.util.List;
-import java.util.Set;
 
 /**
  * <p/>
@@ -183,11 +182,16 @@ public BadInput(String message) {
     }
 
 
-    public static class BadTmpDir extends UserException {
+    public static class BadTempDir extends UserException {
         private static final long serialVersionUID = 0L;
 
-        public BadTmpDir(String message) {
-            super(String.format("Failure working with the tmp directory %s. Override with -Djava.io.tmpdir=X on the command line to a bigger/better file system.  Exact error was %s", System.getProperties().get("java.io.tmpdir"), message));
+        private static final String MESSAGE_FORMAT_STRING = "Failure working with the tmp directory %s. Try changing the tmp dir with with --" + StandardArgumentDefinitions.TMP_DIR_NAME + " on the command line.  Exact error was %s";
+        public BadTempDir(String message) {
+            super(String.format(MESSAGE_FORMAT_STRING, System.getProperties().get("java.io.tmpdir"), message));
+        }
+
+        public BadTempDir(String message, Throwable cause) {
+            super(String.format(MESSAGE_FORMAT_STRING, System.getProperties().get("java.io.tmpdir"), message), cause);
         }
     }
 

src/main/java/org/broadinstitute/hellbender/tools/copynumber/PostprocessGermlineCNVCalls.java

@@ -432,7 +432,7 @@ private void generateSegmentsVCFFileFromAllShards() {
             logger.info("Generating segments VCF file...");
 
             /* perform segmentation */
-            final File pythonScriptOutputPath = IOUtils.tempDir("gcnv-segmented-calls", "");
+            final File pythonScriptOutputPath = IOUtils.createTempDir("gcnv-segmented-calls");
             final boolean pythonScriptSucceeded = executeSegmentGermlineCNVCallsPythonScript(
                     sampleIndex, contigPloidyCallsPath, sortedCallsShardPaths, sortedModelShardPaths,
                     pythonScriptOutputPath);

src/main/java/org/broadinstitute/hellbender/utils/R/RScriptExecutor.java

@@ -83,8 +83,8 @@ public RScriptExecutorException getScriptException(final String message) {
     public boolean exec() {
         List<File> tempFiles = new ArrayList<>();
         try {
-            File tempLibSourceDir  = IOUtils.tempDir("RlibSources.", "");
-            File tempLibInstallationDir = IOUtils.tempDir("Rlib.", "");
+            File tempLibSourceDir  = IOUtils.createTempDir("RlibSources.");
+            File tempLibInstallationDir = IOUtils.createTempDir("Rlib.");
             tempFiles.add(tempLibSourceDir);
             tempFiles.add(tempLibInstallationDir);
 

src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java

@@ -62,38 +62,25 @@ public static boolean isBamFileName(final String inputFileName) {
     }
 
     /**
-     * Creates a temp directory with the prefix and optional suffix.
+     * Creates a temp directory with the given prefix.
      *
-     * @param prefix       Prefix for the directory name.
-     * @param suffix       Optional suffix for the directory name.
-     * @return The created temporary directory.
-     */
-    public static File tempDir(String prefix, String suffix) {
-        return tempDir(prefix, suffix, null);
-    }
-
-    /**
-     * Creates a temp directory with the prefix and optional suffix.
+     * The directory and any contents will be automatically deleted at shutdown.
+     *
+     * This will not work if the temp dir is not representable as a File.
      *
-     * @param prefix        Prefix for the directory name.
-     * @param suffix        Optional suffix for the directory name.
-     * @param tempDirParent Parent directory for the temp directory.
+     * @param prefix       Prefix for the directory name.
      * @return The created temporary directory.
      */
-    public static File tempDir(String prefix, String suffix, File tempDirParent) {
+    public static File createTempDir(String prefix) {
         try {
-            if (tempDirParent == null)
-                tempDirParent = FileUtils.getTempDirectory();
-            if (!tempDirParent.exists() && !tempDirParent.mkdirs())
-                throw new UserException.BadTmpDir("Could not create temp directory: " + tempDirParent);
-            File temp = File.createTempFile(prefix, suffix, tempDirParent);
-            if (!temp.delete())
-                throw new UserException.BadTmpDir("Could not delete sub file: " + temp.getAbsolutePath());
-            if (!temp.mkdir())
-                throw new UserException.BadTmpDir("Could not create sub directory: " + temp.getAbsolutePath());
-            return absolute(temp);
-        } catch (IOException e) {
-            throw new UserException.BadTmpDir(e.getMessage());
+            final File tmpDir = Files.createTempDirectory(prefix)
+                    .normalize()
+                    .toFile();
+            deleteRecursivelyOnExit(tmpDir);
+
+            return tmpDir;
+        } catch (final IOException | SecurityException e) {
+            throw new UserException.BadTempDir(e.getMessage(), e);
         }
     }
 
@@ -120,11 +107,11 @@ public static File writeTempFile(String content, String prefix, String suffix) {
      */
     public static File writeTempFile(String content, String prefix, String suffix, File directory) {
         try {
-            File tempFile = absolute(File.createTempFile(prefix, suffix, directory));
+            File tempFile = File.createTempFile(prefix, suffix, directory).toPath().normalize().toFile();
             FileUtils.writeStringToFile(tempFile, content, StandardCharsets.UTF_8);
             return tempFile;
         } catch (IOException e) {
-            throw new UserException.BadTmpDir(e.getMessage());
+            throw new UserException.BadTempDir(e.getMessage(), e);
         }
     }
 
@@ -157,52 +144,6 @@ else if (file.exists())
         return deleted;
     }
 
-    /**
-     * A mix of getCanonicalFile and getAbsoluteFile that returns the
-     * absolute path to the file without deferencing symbolic links.
-     *
-     * @param file the file.
-     * @return the absolute path to the file.
-     */
-    public static File absolute(File file) {
-        return new File(absolutePath(file));
-    }
-
-    private static String absolutePath(File file) {
-        File fileAbs = file.getAbsoluteFile();
-        LinkedList<String> names = new LinkedList<>();
-        while (fileAbs != null) {
-            String name = fileAbs.getName();
-            fileAbs = fileAbs.getParentFile();
-
-            if (".".equals(name)) {
-                /* skip */
-
-                /* TODO: What do we do for ".."?
-              } else if (name == "..") {
-
-                CentOS tcsh says use getCanonicalFile:
-                ~ $ mkdir -p test1/test2
-                ~ $ ln -s test1/test2 test3
-                ~ $ cd test3/..
-                ~/test1 $
-
-                Mac bash says keep going with getAbsoluteFile:
-                ~ $ mkdir -p test1/test2
-                ~ $ ln -s test1/test2 test3
-                ~ $ cd test3/..
-                ~ $
-
-                For now, leave it and let the shell figure it out.
-                */
-            } else {
-                names.add(0, name);
-            }
-        }
-
-        return ("/" + StringUtils.join(names, "/"));
-    }
-
     /**
      * Writes an embedded resource to a temp file.
      * File is not scheduled for deletion and must be cleaned up by the caller.
@@ -214,7 +155,7 @@ public static File writeTempResource(Resource resource) {
         try {
             temp = File.createTempFile(FilenameUtils.getBaseName(resource.getPath()) + ".", "." + FilenameUtils.getExtension(resource.getPath()));
         } catch (IOException e) {
-            throw new UserException.BadTmpDir(e.getMessage());
+            throw new UserException.BadTempDir(e.getMessage(), e);
         }
         writeResource(resource, temp);
         return temp;

src/main/java/org/broadinstitute/hellbender/utils/test/BaseTest.java

@@ -271,9 +271,7 @@ public static Path getSafeNonExistentPath(final String fileNameWithExtension) {
      * @return an empty directory starting with prefix which will be deleted after the program exits
      */
     public static File createTempDir(final String prefix){
-        final File dir = IOUtils.tempDir(prefix, "");
-        IOUtils.deleteRecursivelyOnExit(dir);
-        return dir;
+        return IOUtils.createTempDir(prefix);
     }
 
     /**

src/test/java/org/broadinstitute/hellbender/utils/io/IOUtilsUnitTest.java

@@ -21,7 +21,7 @@ public final class IOUtilsUnitTest extends GATKBaseTest {
 
     @Test
     public void testTempDir() {
-        File tempDir = IOUtils.tempDir("Q-Unit-Test", "", new File("queueTempDirToDelete"));
+        File tempDir = IOUtils.createTempDir("Q-Unit-Test");
         Assert.assertTrue(tempDir.exists());
         Assert.assertFalse(tempDir.isFile());
         Assert.assertTrue(tempDir.isDirectory());
@@ -30,33 +30,6 @@ public void testTempDir() {
         Assert.assertFalse(tempDir.exists());
     }
 
-    @Test
-    public void testAbsolute() {
-        File dir = IOUtils.absolute(new File("/path/./to/./directory/."));
-        Assert.assertEquals(dir, new File("/path/to/directory"));
-
-        dir = IOUtils.absolute(new File("/"));
-        Assert.assertEquals(dir, new File("/"));
-
-        dir = IOUtils.absolute(new File("/."));
-        Assert.assertEquals(dir, new File("/"));
-
-        dir = IOUtils.absolute(new File("/././."));
-        Assert.assertEquals(dir, new File("/"));
-
-        dir = IOUtils.absolute(new File("/./directory/."));
-        Assert.assertEquals(dir, new File("/directory"));
-
-        dir = IOUtils.absolute(new File("/./directory/./"));
-        Assert.assertEquals(dir, new File("/directory"));
-
-        dir = IOUtils.absolute(new File("/./directory./"));
-        Assert.assertEquals(dir, new File("/directory."));
-
-        dir = IOUtils.absolute(new File("/./.directory/"));
-        Assert.assertEquals(dir, new File("/.directory"));
-    }
-
     @Test
     public void testIsSpecialFile() {
         Assert.assertTrue(IOUtils.isSpecialFile(new File("/dev")));

src/test/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerUnitTest.java

@@ -415,7 +415,7 @@ private static File writeScript(String contents) {
             FileUtils.writeStringToFile(file, contents, StandardCharsets.UTF_8);
             return file;
         } catch (IOException e) {
-            throw new UserException.BadTmpDir(e.getMessage());
+            throw new UserException.BadTempDir(e.getMessage(), e);
         }
     }