search

vaadin / maven-plugin

Vaadin Maven Plug-in
4 stars 18 forks source link

Replace classpath argument with environment variable #73

Closed mcollovati closed 7 years ago

mcollovati commented 7 years ago

If the project has a lot of dependencies compiling widgetset on Windows fails due to too long command line; to reduce command line lenght classpath is given as CLASSPATH environment variable.

This change is Reviewable

CLAassistant commented 7 years ago

CLA assistant check
All committers have signed the CLA.

tsuoanttila commented 7 years ago

I checked this change quickly through and seems ok. I have a couple of questions though and a small request. Is it possible that this change breaks some environments? How many dependencies are we talking about when it breaks? Is there a way to automatically test this somehow?

And the most important request, could you please rebase your change on top of the latest master, so we get a validation build at Travis?

mcollovati commented 7 years ago

I faced the problem on a spring boot monster with more than 370 dependencies (direct and transitive); the command line exceeded the limit of 32k also because the path of project folder was very long (lots of nested folders with long names). For what I understood also a single environment variable has a limit of about 32k, but in older windows versions 32k was the limit of the whole environment block.

https://msdn.microsoft.com/it-it/library/windows/desktop/ms682653(v=vs.85).aspx

I'm not sure if the change can break some environment but I noticed that also surefire plugin uses CLASSPATH environment variable when launching external java process, so I think this would work without problems.

I was able to test the change on the above mentioned project on a windows 7 virtual machine.

Maybe we could add an it test using a pom with a lot of dependencies to huge frameworks (all spring projects for example) and some vaadin addons from directory; also nesting the pom in subfolders could help to reproduce the problem. Could something similar be tested with the actual maven-invoker-plugin configuration? The other problem is that this kind of error only happens on windows systems; could travis setup a similar environment to run the test?

Sorry for bad english

kvalev commented 7 years ago

Is there something that is blocking this pull request? We are experiencing this issue quite a lot and it is very inconvenient to work around it.

hesara commented 7 years ago

Reviewed 1 of 1 files at r1. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

hesara commented 7 years ago

Fixed in master - can consider picking to 7.7 branch, but note that the 8.x version of the plug-in can also be used with Vaadin Framework 7.7 if running with Java 8.