The currently implementation of resource generation is kind of borked. If you attempt to generate a file that exists on the public path, the generated file will "win" in the compiler output folder. However, the shell servlet will always serve the version off of the public path, creating an inconsistency. Another problem with the current system is that files from a previous compilation sitting in the output folder will actually prevent a generator from creating new files.
This patch implements the following proposal:
1) Attempting to generate a resource that's already on the public path fails (null return from tryCreateResource) with a warning.
2) The first generator that attempts to generate a resource during a given compilation will have the opportunity to do so. Any subsequent attempts to generate that resource fail (null return from tryCreateResource) with no warning.
3) A generated file clobbers a file sitting in the output folder.
Notes:
- I added the list of "files generated during this compilation" to CacheManager, since it maintains persistent state and gets a notice when a new compilation is starting.
- I needed a way to pass information about the public files available in the module, so I created a PublicOracle, implemented by ModuleDef, and pass that into the rebind system.
- StandardGeneratorContext would be a cleaner diff if I hadn't renamed the "name" parameter to "partialPath" for consistency with other usages.
- StandardGeneratorContextTest: two of the tests were no longer valid.
Suggested by: jat, bruce
Review by: mmendez
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@1628 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/dev/core/src/com/google/gwt/dev/GWTCompiler.java b/dev/core/src/com/google/gwt/dev/GWTCompiler.java
index b8dabf3..49d031e 100644
--- a/dev/core/src/com/google/gwt/dev/GWTCompiler.java
+++ b/dev/core/src/com/google/gwt/dev/GWTCompiler.java
@@ -115,7 +115,7 @@
private Compilation compilation;
public CompilationRebindOracle() {
- super(typeOracle, propOracle, rules, genDir, outDir, cacheManager);
+ super(typeOracle, propOracle, module, rules, genDir, outDir, cacheManager);
}
/**
@@ -162,7 +162,7 @@
RebindPermutationOracle {
private final StandardRebindOracle rebindOracle = new StandardRebindOracle(
- typeOracle, propOracle, rules, genDir, outDir, cacheManager) {
+ typeOracle, propOracle, module, rules, genDir, outDir, cacheManager) {
/**
* Record generated types.
diff --git a/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java b/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java
index 13940e2..39efffa 100644
--- a/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java
+++ b/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java
@@ -47,7 +47,7 @@
* Represents a module specification. In principle, this could be built without
* XML for unit tests.
*/
-public class ModuleDef {
+public class ModuleDef implements PublicOracle {
/**
* Default to recursive inclusion of java files if no explicit include
* directives are specified.
diff --git a/dev/core/src/com/google/gwt/dev/cfg/PublicOracle.java b/dev/core/src/com/google/gwt/dev/cfg/PublicOracle.java
new file mode 100644
index 0000000..3709608
--- /dev/null
+++ b/dev/core/src/com/google/gwt/dev/cfg/PublicOracle.java
@@ -0,0 +1,39 @@
+/*
+ * Copyright 2007 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.google.gwt.dev.cfg;
+
+import java.net.URL;
+
+/**
+ * Abstracts the process of querying for public files.
+ */
+public interface PublicOracle {
+
+ /**
+ * Finds a file on the public path.
+ *
+ * @param partialPath a file path relative to the root of any public package
+ * @return the url of the file, or <code>null</code> if no such file exists
+ */
+ URL findPublicFile(String partialPath);
+
+ /**
+ * Returns all available public files.
+ *
+ * @return an array containing the partial path to each available public file
+ */
+ String[] getAllPublicFiles();
+}
diff --git a/dev/core/src/com/google/gwt/dev/jdt/CacheManager.java b/dev/core/src/com/google/gwt/dev/jdt/CacheManager.java
index 6c1afda..2cec6ff 100644
--- a/dev/core/src/com/google/gwt/dev/jdt/CacheManager.java
+++ b/dev/core/src/com/google/gwt/dev/jdt/CacheManager.java
@@ -578,6 +578,8 @@
*/
private final Set<String> generatedCupLocations = new HashSet<String>();
+ private final Set<String> generatedResources = new HashSet<String>();
+
private final Mapper identityMapper = new Mapper();
private final Set<String> invalidatedTypes = new HashSet<String>();
@@ -648,6 +650,10 @@
generatedCupLocations.add(generatedCup.getLocation());
}
+ public void addGeneratedResource(String partialPath) {
+ generatedResources.add(partialPath);
+ }
+
/**
* This method returns the <code>TypeOracle</code> associated with this
* <code>CacheManager</code>.
@@ -656,6 +662,10 @@
return oracle;
}
+ public boolean hasGeneratedResource(String partialPath) {
+ return generatedResources.contains(partialPath);
+ }
+
/**
* Ensures that all compilation units generated via generators are removed
* from the system so that they will be generated again, and thereby take into
@@ -668,6 +678,7 @@
iter.remove();
}
}
+ generatedResources.clear();
}
/**
diff --git a/dev/core/src/com/google/gwt/dev/shell/ShellModuleSpaceHost.java b/dev/core/src/com/google/gwt/dev/shell/ShellModuleSpaceHost.java
index 275e82e..a9e79d6 100644
--- a/dev/core/src/com/google/gwt/dev/shell/ShellModuleSpaceHost.java
+++ b/dev/core/src/com/google/gwt/dev/shell/ShellModuleSpaceHost.java
@@ -109,8 +109,8 @@
// It has to wait until now because we need to inject javascript.
//
Rules rules = module.getRules();
- rebindOracle = new StandardRebindOracle(typeOracle, propOracle, rules,
- genDir, outDir, module.getCacheManager());
+ rebindOracle = new StandardRebindOracle(typeOracle, propOracle, module,
+ rules, genDir, outDir, module.getCacheManager());
// Create a completely isolated class loader which owns all classes
// associated with a particular module. This effectively builds a
diff --git a/dev/core/src/com/google/gwt/dev/shell/StandardGeneratorContext.java b/dev/core/src/com/google/gwt/dev/shell/StandardGeneratorContext.java
index 6654569..f31780a 100644
--- a/dev/core/src/com/google/gwt/dev/shell/StandardGeneratorContext.java
+++ b/dev/core/src/com/google/gwt/dev/shell/StandardGeneratorContext.java
@@ -23,6 +23,7 @@
import com.google.gwt.core.ext.typeinfo.JClassType;
import com.google.gwt.core.ext.typeinfo.NotFoundException;
import com.google.gwt.core.ext.typeinfo.TypeOracle;
+import com.google.gwt.dev.cfg.PublicOracle;
import com.google.gwt.dev.jdt.CacheManager;
import com.google.gwt.dev.jdt.StaticCompilationUnitProvider;
import com.google.gwt.dev.jdt.TypeOracleBuilder;
@@ -32,7 +33,6 @@
import java.io.ByteArrayOutputStream;
import java.io.CharArrayWriter;
import java.io.File;
-import java.io.IOException;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.net.MalformedURLException;
@@ -125,23 +125,19 @@
*/
private static class PendingResource {
- private final File pendingFile;
private final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ private final String partialPath;
+ private final File pendingFile;
- public PendingResource(File pendingFile) {
- this.pendingFile = pendingFile;
+ public PendingResource(File outDir, String partialPath) {
+ this.partialPath = partialPath;
+ this.pendingFile = new File(outDir, partialPath);
}
public void commit(TreeLogger logger) throws UnableToCompleteException {
logger = logger.branch(TreeLogger.TRACE, "Writing generated resource '"
+ pendingFile.getAbsolutePath() + "'", null);
- if (pendingFile.exists()) {
- logger.log(TreeLogger.ERROR,
- "The destination file already exists; aborting the commit", null);
- throw new UnableToCompleteException();
- }
-
Util.writeBytesToFile(logger, pendingFile, baos.toByteArray());
}
@@ -153,30 +149,8 @@
return baos;
}
- public boolean isSamePath(TreeLogger logger, File other) {
- File failedFile = null;
- try {
- /*
- * We try to convert both files to their canonical form. Either one
- * might throw an exception, so we keep track of the one being converted
- * so that we can say which one failed in the case of an error.
- */
- failedFile = pendingFile;
- File thisFile = pendingFile.getCanonicalFile();
- failedFile = pendingFile;
- File thatFile = other.getCanonicalFile();
-
- if (thisFile.equals(thatFile)) {
- return true;
- } else {
- return false;
- }
- } catch (IOException e) {
- logger.log(TreeLogger.ERROR,
- "Unable to determine canonical path of pending resource '"
- + failedFile.toString() + "'", e);
- }
- return false;
+ public String getPartialPath() {
+ return partialPath;
}
}
@@ -194,6 +168,8 @@
private final PropertyOracle propOracle;
+ private final PublicOracle publicOracle;
+
private final TypeOracle typeOracle;
private final Map<PrintWriter, GeneratedCompilationUnitProvider> uncommittedGeneratedCupsByPrintWriter = new IdentityHashMap<PrintWriter, GeneratedCompilationUnitProvider>();
@@ -203,10 +179,11 @@
* available in the supplied type oracle although it isn't strictly required.
*/
public StandardGeneratorContext(TypeOracle typeOracle,
- PropertyOracle propOracle, File genDir, File outDir,
- CacheManager cacheManager) {
- this.propOracle = propOracle;
+ PropertyOracle propOracle, PublicOracle publicOracle, File genDir,
+ File outDir, CacheManager cacheManager) {
this.typeOracle = typeOracle;
+ this.propOracle = propOracle;
+ this.publicOracle = publicOracle;
this.genDir = genDir;
this.outDir = outDir;
this.cacheManager = cacheManager;
@@ -235,6 +212,7 @@
if (pendingResource != null) {
// Actually write the bytes to disk.
pendingResource.commit(logger);
+ cacheManager.addGeneratedResource(pendingResource.getPartialPath());
// The resource is now no longer pending, so remove it from the map.
// If the commit above throws an exception, it's okay to leave the entry
@@ -373,21 +351,21 @@
return gcup.pw;
}
- public OutputStream tryCreateResource(TreeLogger logger, String name)
+ public OutputStream tryCreateResource(TreeLogger logger, String partialPath)
throws UnableToCompleteException {
logger = logger.branch(TreeLogger.DEBUG,
- "Preparing pending output resource '" + name + "'", null);
+ "Preparing pending output resource '" + partialPath + "'", null);
// Disallow null or empty names.
- if (name == null || name.trim().equals("")) {
+ if (partialPath == null || partialPath.trim().equals("")) {
logger.log(TreeLogger.ERROR,
"The resource name must be a non-empty string", null);
throw new UnableToCompleteException();
}
// Disallow absolute paths.
- File f = new File(name);
+ File f = new File(partialPath);
if (f.isAbsolute()) {
logger.log(
TreeLogger.ERROR,
@@ -397,7 +375,7 @@
}
// Disallow backslashes (to promote consistency in calling code).
- if (name.indexOf('\\') >= 0) {
+ if (partialPath.indexOf('\\') >= 0) {
logger.log(
TreeLogger.ERROR,
"Resource paths must contain forward slashes (not backslashes) to denote subdirectories",
@@ -405,28 +383,31 @@
throw new UnableToCompleteException();
}
- // Compute the final path.
- File pendingFile = new File(outDir, name);
+ // Check for public path collision.
+ if (publicOracle.findPublicFile(partialPath) != null) {
+ logger.log(TreeLogger.WARN, "Cannot create resource '" + partialPath
+ + "' because it already exists on the public path", null);
+ return null;
+ }
- // See if this file is already pending.
+ // See if the file is already committed.
+ if (cacheManager.hasGeneratedResource(partialPath)) {
+ return null;
+ }
+
+ // See if the file is pending.
for (Iterator<PendingResource> iter = pendingResourcesByOutputStream.values().iterator(); iter.hasNext();) {
PendingResource pendingResource = iter.next();
- if (pendingResource.isSamePath(logger, pendingFile)) {
+ if (pendingResource.getPartialPath().equals(partialPath)) {
// It is already pending.
- logger.log(TreeLogger.WARN, "The file is already a pending resource",
- null);
+ logger.log(TreeLogger.WARN, "The file '" + partialPath
+ + "' is already a pending resource", null);
return null;
}
}
- // If this file already exists, we won't overwrite it.
- if (pendingFile.exists()) {
- logger.log(TreeLogger.TRACE, "File already exists", null);
- return null;
- }
-
// Record that this file is pending.
- PendingResource pendingResource = new PendingResource(pendingFile);
+ PendingResource pendingResource = new PendingResource(outDir, partialPath);
OutputStream os = pendingResource.getOutputStream();
pendingResourcesByOutputStream.put(os, pendingResource);
diff --git a/dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java b/dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java
index 2c1f8ad..fbb237b 100644
--- a/dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java
+++ b/dev/core/src/com/google/gwt/dev/shell/StandardRebindOracle.java
@@ -20,6 +20,7 @@
import com.google.gwt.core.ext.UnableToCompleteException;
import com.google.gwt.core.ext.typeinfo.JClassType;
import com.google.gwt.core.ext.typeinfo.TypeOracle;
+import com.google.gwt.dev.cfg.PublicOracle;
import com.google.gwt.dev.cfg.Rule;
import com.google.gwt.dev.cfg.Rules;
import com.google.gwt.dev.cfg.StaticPropertyOracle;
@@ -50,9 +51,10 @@
private final List<String> usedTypeNames = new ArrayList<String>();
- public Rebinder(TypeOracle typeOracle, PropertyOracle propOracle) {
- genCtx = new StandardGeneratorContext(typeOracle, propOracle, genDir,
- outDir, cacheManager);
+ public Rebinder(TypeOracle typeOracle, PropertyOracle propOracle,
+ PublicOracle publicOracle) {
+ genCtx = new StandardGeneratorContext(typeOracle, propOracle,
+ publicOracle, genDir, outDir, cacheManager);
}
public String rebind(TreeLogger logger, String typeName)
@@ -139,14 +141,18 @@
private final PropertyOracle propOracle;
+ private final PublicOracle publicOracle;
+
private final Rules rules;
private final TypeOracle typeOracle;
public StandardRebindOracle(TypeOracle typeOracle, PropertyOracle propOracle,
- Rules rules, File genDir, File moduleOutDir, CacheManager cacheManager) {
+ PublicOracle publicOracle, Rules rules, File genDir, File moduleOutDir,
+ CacheManager cacheManager) {
this.typeOracle = typeOracle;
this.propOracle = propOracle;
+ this.publicOracle = publicOracle;
this.rules = rules;
this.genDir = genDir;
this.outDir = moduleOutDir;
@@ -158,10 +164,11 @@
}
public StandardRebindOracle(TypeOracle typeOracle,
- StaticPropertyOracle propOracle, Rules rules, File genDir,
- File moduleOutDir) {
+ StaticPropertyOracle propOracle, PublicOracle publicOracle, Rules rules,
+ File genDir, File moduleOutDir) {
// This is a path used for non-hosted mode execution; therefore no caching.
- this(typeOracle, propOracle, rules, genDir, moduleOutDir, null);
+ this(typeOracle, propOracle, publicOracle, rules, genDir, moduleOutDir,
+ null);
}
public String rebind(TreeLogger logger, String typeName)
@@ -169,7 +176,7 @@
logger = Messages.TRACE_TOPLEVEL_REBIND.branch(logger, typeName, null);
- Rebinder rebinder = new Rebinder(typeOracle, propOracle);
+ Rebinder rebinder = new Rebinder(typeOracle, propOracle, publicOracle);
String result = rebinder.rebind(logger, typeName);
Messages.TRACE_TOPLEVEL_REBIND_RESULT.log(logger, result, null);
diff --git a/dev/core/test/com/google/gwt/dev/shell/StandardGeneratorContextTest.java b/dev/core/test/com/google/gwt/dev/shell/StandardGeneratorContextTest.java
index 33d9053..e3ee325 100644
--- a/dev/core/test/com/google/gwt/dev/shell/StandardGeneratorContextTest.java
+++ b/dev/core/test/com/google/gwt/dev/shell/StandardGeneratorContextTest.java
@@ -20,6 +20,7 @@
import com.google.gwt.core.ext.TreeLogger;
import com.google.gwt.core.ext.UnableToCompleteException;
import com.google.gwt.core.ext.typeinfo.TypeOracle;
+import com.google.gwt.dev.cfg.PublicOracle;
import com.google.gwt.dev.jdt.CacheManager;
import com.google.gwt.dev.util.Util;
@@ -29,6 +30,9 @@
import java.io.File;
import java.io.IOException;
import java.io.OutputStream;
+import java.io.UnsupportedEncodingException;
+import java.net.MalformedURLException;
+import java.net.URL;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
@@ -48,6 +52,25 @@
}
}
+ private static class MockPublicOracle implements PublicOracle {
+
+ public URL findPublicFile(String partialPath) {
+ if ("onPublicPath.txt".equals(partialPath)) {
+ try {
+ return new File("").toURL();
+ } catch (MalformedURLException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ return null;
+ }
+
+ public String[] getAllPublicFiles() {
+ return new String[] {"onPublicPath.txt"};
+ }
+
+ }
+
private static class MockTreeLogger implements TreeLogger {
public TreeLogger branch(Type type, String msg, Throwable caught) {
@@ -72,6 +95,7 @@
private final List<File> toDelete = new ArrayList<File>();
private final TypeOracle mockTypeOracle = new MockTypeOracle();
private final PropertyOracle mockPropOracle = new MockPropertyOracle();
+ private final PublicOracle mockPublicOracle = new MockPublicOracle();
private final File tempGenDir;
private final File tempOutDir;
private final CacheManager mockCacheManager = new MockCacheManager();
@@ -83,7 +107,7 @@
tempGenDir = createTempDir("gwt-gen-");
tempOutDir = createTempDir("gwt-out-");
genCtx = new StandardGeneratorContext(mockTypeOracle, mockPropOracle,
- tempGenDir, tempOutDir, mockCacheManager);
+ mockPublicOracle, tempGenDir, tempOutDir, mockCacheManager);
}
public void testTryCreateResource_badFileName() {
@@ -204,6 +228,21 @@
assertNull(os2);
}
+ public void testTryCreateResource_duplicateCreationAfterCommit()
+ throws UnableToCompleteException, UnsupportedEncodingException,
+ IOException {
+ String path = createTempOutFilename();
+ OutputStream os1 = genCtx.tryCreateResource(mockLogger, path);
+ os1.write("going to call commit twice after this...".getBytes(Util.DEFAULT_ENCODING));
+ genCtx.commitResource(mockLogger, os1);
+ File createdFile = new File(tempOutDir, path);
+ assertTrue(createdFile.exists());
+ rememberToDelete(createdFile);
+
+ OutputStream os2 = genCtx.tryCreateResource(mockLogger, path);
+ assertNull(os2);
+ }
+
public void testTryCreateResource_finishCalledTwice()
throws UnableToCompleteException, IOException {
// Borrow impl.
@@ -231,61 +270,23 @@
}
/**
- * Tests that tryCreateResource() throws an exception when commit() is called
- * if the output exists. This situation should only happen if the file is
- * created externally between the time tryCreateResource() is called and
- * commit() is called. In the normal case of an existing file,
- * tryCreateResource() would return <code>null</code> so there would be
- * nothing to commit.
+ * Tests that tryCreateResource() returns <code>null</code> when the
+ * specified file is already on the public path.
*
* @throws UnableToCompleteException
* @throws IOException
*/
- public void testTryCreateResource_outputFileConflictAtCommit()
- throws UnableToCompleteException, IOException {
- String path = createTempOutFilename();
-
- OutputStream os = genCtx.tryCreateResource(mockLogger, path);
- assertNotNull(
- "This test requires that the file being created does not already exist at this point",
+ public void testTryCreateResource_outputFileOnPublicPath()
+ throws UnableToCompleteException {
+ OutputStream os = genCtx.tryCreateResource(mockLogger, "onPublicPath.txt");
+ assertNull(
+ "tryCreateResource() should return null when the target file is already on the public path",
os);
- byte[] arrayWritten = new byte[] {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
- os.write(arrayWritten);
- try {
- // Manually create the file that would normally be created by commit().
- File existingFile = new File(tempOutDir, path);
- Util.writeStringAsFile(existingFile, "please don't clobber me");
- assertTrue(existingFile.exists());
- rememberToDelete(existingFile);
-
- genCtx.commitResource(mockLogger, os);
- fail("The previous statement should have caused an exception since writing the resource must have failed");
- } catch (UnableToCompleteException e) {
- // Success.
- }
}
- /**
- * Tests that tryCreateResource() returns <code>null</code> when the
- * specified file already exists.
- *
- * @throws UnableToCompleteException
- * @throws IOException
- */
- public void testTryCreateResource_outputFileConflictAtCreation()
- throws UnableToCompleteException, IOException {
- String path = createTempOutFilename();
-
- // Manually create the file that would normally be created by commit().
- File existingFile = new File(tempOutDir, path);
- Util.writeStringAsFile(existingFile, "please don't clobber me");
- assertTrue(existingFile.exists());
- rememberToDelete(existingFile);
-
- OutputStream os = genCtx.tryCreateResource(mockLogger, path);
- assertNull(
- "tryCreateResource() should return null when the target file already exists",
- os);
+ @Override
+ protected void setUp() throws Exception {
+ mockCacheManager.invalidateVolatileFiles();
}
protected void tearDown() throws Exception {
@@ -297,7 +298,6 @@
private File createTempDir(String prefix) {
String baseTempPath = System.getProperty("java.io.tmpdir");
- File baseTempDir = new File(baseTempPath);
File newTempDir;
do {
newTempDir = new File(baseTempPath, prefix + System.currentTimeMillis());
