[gwt-contrib] Re: The logic to avoid rewriting compilation units into archived units failed in some build environm... (issue1518803)

2011-08-11 Thread jbrosenberg 

LGTM w/a comment nit


http://gwt-code-reviews.appspot.com/1518803/diff/3002/dev/core/src/com/google/gwt/dev/CompileModule.java
File dev/core/src/com/google/gwt/dev/CompileModule.java (right):

http://gwt-code-reviews.appspot.com/1518803/diff/3002/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode194
dev/core/src/com/google/gwt/dev/CompileModule.java:194: Map> unitsInArchives = new HashMap>();
Ok, so can you make it a bit more clear in this comment, so we know why
it's relevant to say "they may not have been written to the
classpath..."?
Are you saying that archive files are written to the classpath, and then
reloaded from the classpath, in the same process?  Or is it always a
matter of writing things so that the next process (with a new
classloader) will find them? In which case it make more sense to refer
to in that way (since how do I know a subsequent process will even have
the same classpath as the current invocation)?

http://gwt-code-reviews.appspot.com/1518803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: The logic to avoid rewriting compilation units into archived units failed in some build environm... (issue1518803)

2011-08-11 Thread zundel 


http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java
File dev/core/src/com/google/gwt/dev/CompileModule.java (right):

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode194
dev/core/src/com/google/gwt/dev/CompileModule.java:194: Map> unitsInArchives = new HashMap>();
On 2011/08/11 15:29:46, jbrosenberg wrote:

...
What's the significance of units being written to the classpath?


Forgot to address this one.  A module looks on the classpath to find
.gwt.xml files,  when it does, it also looks for a corrsponding .gwtar
file.  In a normal build, some earlier program put the .gwtar file there
and it was there when dev mode or the compiler started, but when you are
running CompileModule itself, you are creating these new files.

In the normal ant build, the .gwtar files are directly output into the
classpath, so that you can read them back when the next module is
compiled.  But some build environments write out the .gwtar file to a
temporary spot, then when the entire CompileModule is done, it moves the
.gwtar files onto the classpath for the next task in the build.
CompileModule wasn't smart enough to handle that second case
efficiently, and compiled each module as if there were no nested modules
already compiled.

http://gwt-code-reviews.appspot.com/1518803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: The logic to avoid rewriting compilation units into archived units failed in some build environm... (issue1518803)

2011-08-11 Thread zundel 


http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java
File dev/core/src/com/google/gwt/dev/CompileModule.java (right):

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode194
dev/core/src/com/google/gwt/dev/CompileModule.java:194: Map> unitsInArchives = new HashMap>();
On 2011/08/11 15:29:46, jbrosenberg wrote:

I'm a little confused by this comment.
When you say "session", is it referring specifically to a dev mode

session?  Or

could it also apply to a web-mode compile?  In which case what's a

session?

What I mean is an invocation of CompileModule, which is a standalone
program.  Updated to make that more clear.



What's the significance of units being written to the classpath?


http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode197
dev/core/src/com/google/gwt/dev/CompileModule.java:197: // modules
compiled in the same session.
On 2011/08/11 15:29:46, jbrosenberg wrote:

maybe a more specific name ("newlyCompiledModules")?


Done.

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode231
dev/core/src/com/google/gwt/dev/CompileModule.java:231:
On 2011/08/11 15:29:46, jbrosenberg wrote:

session?


Done.

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode238
dev/core/src/com/google/gwt/dev/CompileModule.java:238:
On 2011/08/11 15:29:46, jbrosenberg wrote:

Is this really the right comment for this entire for loop?


Done.

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java
File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right):

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode137
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:137: new
LinkedHashMap>();
On 2011/08/11 15:29:46, jbrosenberg wrote:

whitespace?  Here and below?


Done.

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java
File dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java (right):

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java#newcode232
dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:232: }
On 2011/08/11 15:29:46, jbrosenberg wrote:

whitespace?


Done.

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java#newcode241
dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:241: }
On 2011/08/11 15:29:46, jbrosenberg wrote:

will this add inherited modules from multiple levels in the hierarchy?

Or only

first level inheritance?


nestedLoad is invoked recursively, if that's what you mean.


It seems like it would be relevant to detecting
whether a module's compilation units are already loaded, etc.


Essentially, I just moved a Set<> out of ModuleDefLoader we were keeping
into ModuleDef because I wanted to be able to query the set after the
module had been loaded to fix the problem in CompileModule.

Compile Module wants to exclude units from an archive if they are a part
of an inherited module, but the inheritance information was lost after
load time.

http://gwt-code-reviews.appspot.com/1518803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: The logic to avoid rewriting compilation units into archived units failed in some build environm... (issue1518803)

2011-08-11 Thread jbrosenberg 


http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java
File dev/core/src/com/google/gwt/dev/CompileModule.java (right):

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode194
dev/core/src/com/google/gwt/dev/CompileModule.java:194: Map> unitsInArchives = new HashMap>();
I'm a little confused by this comment.
When you say "session", is it referring specifically to a dev mode
session?  Or could it also apply to a web-mode compile?  In which case
what's a session?
What's the significance of units being written to the classpath?

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode197
dev/core/src/com/google/gwt/dev/CompileModule.java:197: // modules
compiled in the same session.
maybe a more specific name ("newlyCompiledModules")?

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode231
dev/core/src/com/google/gwt/dev/CompileModule.java:231:
session?

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode238
dev/core/src/com/google/gwt/dev/CompileModule.java:238:
Is this really the right comment for this entire for loop?

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java
File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right):

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode137
dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:137: new
LinkedHashMap>();
whitespace?  Here and below?

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java
File dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java (right):

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java#newcode232
dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:232: }
whitespace?

http://gwt-code-reviews.appspot.com/1518803/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java#newcode241
dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:241: }
will this add inherited modules from multiple levels in the hierarchy?
Or only first level inheritance?  It seems like it would be relevant to
detecting whether a module's compilation units are already loaded, etc.

http://gwt-code-reviews.appspot.com/1518803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors