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

[#1823] Fix play.exceptions.CompilationException #786

Merged
merged 2 commits into from
Aug 8, 2014

Conversation

xael-fry
Copy link
Member

@xael-fry xael-fry commented Aug 4, 2014

@cloudbees-pull-request-builder

play-1-3-x-pull-requests #243 FAILURE
Looks like there's a problem with this pull request

@xael-fry
Copy link
Member Author

xael-fry commented Aug 4, 2014

@Notalifeform could you have a look on my PR (some tests to check if there isn't any problem on mac, forx ex)?

@cloudbees-pull-request-builder

play-1-3-x-pull-requests #244 FAILURE
Looks like there's a problem with this pull request

// 1. check if there is a folder (without extension)
VirtualFile javaFile = path.child(fileOrDir);

if (javaFile.exists() && javaFile.isDirectory() && javaFile.matchName(fileName)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don´t know who that came into it. My original PR was
if (javaFile.exists() && javaFile.isDirectory()) {
return null; // we found a package
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but as I said there was still problem with your PR, your PR only work with play precompile.
example with name = controllers.Application, it was sometimes detect as a directory on windows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is than, that the name of a directory must be cheched as well by its canonical name. My implementation was base on the assumption, that java packages must be allways lower case and i do not have to check the directory for case insensitivity. I will update my pull request.
You tested this with the normal "play run" on the small sample application from #786 1823 lighthouse ticket?
synapplix@e76ded2

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I used the project sample attached to the issue, I run also the ApplicationTest as a Junit Test. The function matchName just do waht you said : directory "must be cheched as well by its canonical name".
So I think my change are ok, aren't they?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no it is not I think on this line it must be:
javaFile.matchName(fileOrDir)
and not
javaFile.matchName(fileName)

@flybyray
Copy link
Contributor

flybyray commented Aug 4, 2014

Nevertheless with this patch we have a new problem with the template code itself.
You can run the simple testapp ( like in http://play.lighthouseapp.com/projects/57987/tickets/1823-windows-support-eclipse-jdt-compiler ). But if you call the controller action for the subpackage Application http://localhost:9000/application.Application/index you will get a template not found message.
On Windows you will need to create a "Application" directory within the existing "Application" directory:
\testapp\app\views\Application\index.html
\testapp\app\views\Application\Application\index.html

On Linux it is possible to create two different directories "application" and "Application":
\testapp\app\views\Application\index.html
\testapp\app\views\application\Application\index.html

Perhaps a other naming schema for templates would be better something like this:
\testapp\app\views\Application.index.html
\testapp\app\views\application\Application.index.html

@xael-fry
Copy link
Member Author

xael-fry commented Aug 5, 2014

We cannot change the naming schema for templates, I thinks the simpliest solution to make it work on windows is not to user the render function but use renderTemplate instead

…ion error

[#1823] Some fix on the method getJava
@cloudbees-pull-request-builder

play-1-3-x-pull-requests #246 FAILURE
Looks like there's a problem with this pull request

@flybyray
Copy link
Contributor

flybyray commented Aug 5, 2014

How is it possible to see the actual failure of this build? If I click on the link i only see a Login screen from CloudBees. Is there any chance to get the build script to try the same on our own jenkins server (i hate those restrictive build services, hope you will leave it soon)?

@xael-fry
Copy link
Member Author

xael-fry commented Aug 5, 2014

Yes I saw that since some update from cloudee, anonymous cannot access build, I try to male make it accessible.
The problem is :

[exec] 02:53:14,223 TRACE ~ Plugin play.plugins.ConfigurablePluginD~ The application does not start. There are errors: java.io.IOException: Server returned HTTP response code: 500 for URL: http://localhost:9003/@tests.list

We have it for all PR base on the 1.3.x branch, but I don't know why, as I didn't have this issue on my computer.

@Notalifeform
Copy link
Contributor

@flybyray anonymous access to cloudbees has been restored
@xael-fry all test for this branch pass now on OSX

xael-fry added a commit that referenced this pull request Aug 8, 2014
[#1823] Fix play.exceptions.CompilationException
@xael-fry xael-fry merged commit 2d3343a into playframework:1.3.x Aug 8, 2014
@xael-fry xael-fry mentioned this pull request Aug 8, 2014
@xael-fry xael-fry deleted the lighthouse-1823-patch branch October 26, 2015 06:24
flybyray added a commit to flybyray/play1 that referenced this pull request Mar 8, 2018
xael-fry added a commit that referenced this pull request Mar 16, 2018
[#1823] Fix play.exceptions.CompilationException (fix for the fix #786)
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 this pull request may close these issues.

4 participants