Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The temporary class of the template is output directly under the application dir. #1354

Closed
rakix opened this issue Apr 16, 2021 · 15 comments · Fixed by #1368
Closed

The temporary class of the template is output directly under the application dir. #1354

rakix opened this issue Apr 16, 2021 · 15 comments · Fixed by #1368
Milestone

Comments

@rakix
Copy link

rakix commented Apr 16, 2021

Are you looking for help?

This is an issue tracker, used to manage and track the development of Play. It is not a support system and so it is not a place to ask questions or get help. If you're not sure if you have found a bug, or if you have a feature request, the best place to start is with either the Discuss Play Forum or Stack Overflow.

Play Version (1.5.x / etc)

1.6.0

Operating System (Ubuntu 15.10 / MacOS 10.10 / Windows 10)

windows/Centos

JDK (Oracle 1.8.0_72, OpenJDK 1.8.x, Azul Zing)

OpenJDK 11

The temporary class of the groovy template will be output directly under the application dir.
I want them to be output to the tmp dir.

@@ -118,6 +118,7 @@ public class GroovyTemplate extends BaseTemplate {
     protected CompilerConfiguration setUpCompilerConfiguration() {
         CompilerConfiguration compilerConfiguration = new CompilerConfiguration();
         compilerConfiguration.setSourceEncoding("utf-8"); // ouf
+        compilerConfiguration.setTargetDirectory("tmp");
         return compilerConfiguration;
     }
@JohannesBeranek
Copy link

Same issue here. Comparing with 1.5.3 it seems that the files weren't written to disk before, not even in the tmp directory, or they were cleaned up afterwards again. Anyway I did not find them when running the same Project in 1.5.3 and doing exactly the same things. Also, they only seem to get created when a stack trace or error of some sort is created. I can trigger creation of those files when hitting an unknown route (notFound exception) in dev mode for example, but nothing seems to happen until then.

@pgarr
Copy link

pgarr commented Apr 22, 2021

Reason is most likely #1338 PR, where Groovy was upgraded and GroovyTemplate.java changed.
I changed setUpCompilerConfiguration method, so files are saved under tmpDir

protected CompilerConfiguration setUpCompilerConfiguration() {
      CompilerConfiguration compilerConfiguration = new CompilerConfiguration();
      compilerConfiguration.setSourceEncoding("utf-8"); // ouf
      File subDirectory = new File(Play.tmpDir, "templates");
      compilerConfiguration.setTargetDirectory(subDirectory);
      return compilerConfiguration;
 }

But, in earlier version, there was some kind of overriding OUTPUT phase from groovy CompilationUnit class, where these files are created:

 Field phasesF = compilationUnit.getClass().getDeclaredField("phaseOperations");
 phasesF.setAccessible(true);
 LinkedList[] phases = (LinkedList[]) phasesF.get(compilationUnit);
 LinkedList<GroovyClassOperation> output = new LinkedList<>();
 phases[Phases.OUTPUT] = output;
 output.add(new GroovyClassOperation() {
                   @Override
                   public void call(GroovyClass gclass) {
                       groovyClassesForThisTemplate.add(gclass);
                   }
});

Maybe it would be better to restore this functionality.

@JohannesBeranek
Copy link

It's also possible to just set system property groovy.target.directory in e.g. application initialization, so that's what I did. Benefit is I didn't have to alter Play sources.
Also found what you posted @pgarr but I wonder what exactly those are written for, because apparently it worked without issues before. Seems to me like the new version is also considerably slower in the initial run, but I've found various bug reports on the groovy github regarding that where groovy 3 was about 5 times slower in compiling templates than groovy 2.5 in some places. Hopefully that will get fixed soon though, which would be a good time to update the play dependency again as well.

@JohannesBeranek
Copy link

JohannesBeranek commented Apr 23, 2021

Links to the groovy performance related issues:
https://issues.apache.org/jira/browse/GROOVY-9588

supposedly fixed in 3.0.5, but then later in 3.0.7 we have:
https://issues.apache.org/jira/browse/GROOVY-9964

And I can confirm that in my project groovy 3 is also considerably slower than groovy 2 in terms of compilation time, both with play groovy 3.0.6 and an update 3.0.8

(Edit: I know this doesn't look like it's directly related to the ticket, but it might be - maybe writing out those temp files is also part why this is actually being slower in groovy 3 than in groovy 2)

@JohannesBeranek
Copy link

JohannesBeranek commented Apr 26, 2021

It seems that even with the system property groovy.target.directory set, the compiled template of the 404 page still sometimes gets saved to the root of the application. Might actually work better if you use ApplicationPlugin :: onLoad. Nevertheless would be good to know what those files are actually for and if they are needed, and if not, if we can remove that output stage again.

@sbeigel
Copy link
Contributor

sbeigel commented Sep 11, 2021

Same problem here. Looking at the source of GroovyTemplate you can see some comments explaining the change (starting line 143). I changed this back to the previous behavior by removing lines 151-156 and re-adding (un-commenting) lines 159-169 and everything seems to be back to normal... It would be great if you all could check this as well so we can create a PR to revert this.

@cies
Copy link
Contributor

cies commented Nov 6, 2021

@sbeigel Verified that the steps you describe fix the Template_* root dir pollution.

Maximillian13 added a commit to Maximillian13/play1 that referenced this issue Dec 27, 2021
…o fix template files saving to projects root
@Maximillian13
Copy link
Contributor

@sbeigel I Verified that the steps you describe fix the Template_* root dir pollution as well. I made a pull request reverting to the old behavior.

@tazmaniax
Copy link
Collaborator

Yes the previous change should be reverted and I've left a comment on #1368 to add a comment in the code to avoid this change being made again in the future.

Maximillian13 added a commit to Maximillian13/play1 that referenced this issue Dec 28, 2021
@Maximillian13
Copy link
Contributor

I added the comments but now the Travis CI test is failing. This is my first real pull request to a public repo so its possible I messed up on pushing to the branch.
I just ran
git commit -m "[#1354] Add comment explaining revert"
git push origin github-issue-1354-patch

Do I need to pull on this branch first or something?

Maximillian13 added a commit to Maximillian13/play1 that referenced this issue Jan 4, 2022
@Maximillian13
Copy link
Contributor

Ok looks like everything is passing now. Im not sure the process. Do I leave this pull request up and it will get looked at eventually?

@tazmaniax
Copy link
Collaborator

@Maximillian13 good stuff, thx!

@asolntsev do you have any comments on this?

@xael-fry can you release if you're ok with it?

xael-fry added a commit that referenced this issue Jan 28, 2022
[#1354] Revert experimental GroovyTempate.compile code to fix template issue
@xael-fry xael-fry added this to the 1.7.0 milestone Jan 28, 2022
@xael-fry
Copy link
Member

Merged thanks

@holajsh
Copy link
Contributor

holajsh commented Apr 14, 2022

It seems that the culprit code has been reintroduced in Play 1.7.0.
The framework is again trying to write to some place where it will not have permissions to do so.
It tried the modification of setUpCompilerConfiguration proposed by @pgarr which seems to work fine but I don't have all the implications of such a change so I guess that the code should be restored as it was before with the ugly modification of phaseOperations using reflection.

@holajsh
Copy link
Contributor

holajsh commented Apr 14, 2022

I guess this issue should be reopened but I can't do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants