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

Build fails when building in parallel #1948

Closed
folker-kuhn opened this issue Oct 5, 2021 · 13 comments · Fixed by #2006
Closed

Build fails when building in parallel #1948

folker-kuhn opened this issue Oct 5, 2021 · 13 comments · Fixed by #2006
Assignees
Milestone

Comments

@folker-kuhn
Copy link
Contributor

Building Piranha with mvn clean install -T8 results in errors. mvn clean install without -T8 works, but takes like 10 minutes in my system.

Can you fix the errors during the parallel build? Thanks!

@Thihup
Copy link
Collaborator

Thihup commented Oct 5, 2021

Hi @folker-kuhn!
We have some integration tests that currently can't run in parallel. What you could do is skip the tests and the Javadoc, as most of the build time is because of them.
mvn clean install -T8 -DskipTests -Dmaven.javadoc.skip=true

@folker-kuhn
Copy link
Contributor Author

Oh, that is not a bad idea, thanks!

I think this fixes my problem, but out of the box parallel would be good to help others, not? Why dont integration test can run in parallel? Can this be fixed?

@Thihup
Copy link
Collaborator

Thihup commented Oct 20, 2021

I did a quick look in what needs to be done:

  • Use different ports for the HTTP tests (using a library like https://github.com/alexpanov/freeportfinder could help here)
  • Add a dependency on Piranha Server and Micro on some tests (/test/server/pom.xml and /test/micro/pom.xml) to make sure they start only after the project has built it

Unfortunately, we still need a single run to install the artifacts in the local Maven repo to Arquillian find the dependencies.

Other than that, it seems possible.

@arjantijms
Copy link
Collaborator

That sounds cool to do. Especially fixing the dependency parts has allowed the GlassFish project to build in parallel out of the box.

Thihup pushed a commit to Thihup/piranha that referenced this issue Oct 23, 2021
Thihup pushed a commit to Thihup/piranha that referenced this issue Oct 23, 2021
This make sure that both projects have already built before running
the tests in parallel
Thihup added a commit to Thihup/piranha that referenced this issue Oct 23, 2021
Thihup added a commit to Thihup/piranha that referenced this issue Oct 23, 2021
This make sure that both projects have already built before running
the tests in parallel
@mnriem
Copy link
Contributor

mnriem commented Oct 23, 2021

Nice! What is the speedup you are seeing?

Thihup added a commit to Thihup/piranha that referenced this issue Oct 23, 2021
Thihup added a commit to Thihup/piranha that referenced this issue Oct 23, 2021
This make sure that both projects have already built before running
the tests in parallel
Thihup added a commit to Thihup/piranha that referenced this issue Oct 23, 2021
@Thihup
Copy link
Collaborator

Thihup commented Oct 23, 2021

@mnriem In my machine, a full build (mvn clean install) it went down from about 14 minutes to 2 minutes (using mvn clean install -T8).

Sometimes, however, we get a pretty confusing error, like this one:
cloud.piranha.test.faces.mojarra.Mojarra2Test.txt

It seems that it reads an invalid zip file, so I need to take a closer look. But otherwise, the improvement seems to be pretty good.

@arjantijms
Copy link
Collaborator

That looks pretty fantastic indeed. Building in parallel (without the tests) already gave me a huge speed boost here; 24 seconds (no tests, parallel) vs 7 minutes (tests, serial).

@mnriem
Copy link
Contributor

mnriem commented Oct 23, 2021

My gut on the confusing error would be that say process A is trying to read the signature of a given JAR/ZIP and say process B is updating the same file ... you probably want to add some additional logging so it shows you which JAR/ZIP file is tripping you up?

@mnriem
Copy link
Contributor

mnriem commented Oct 29, 2021

@Thihup Do you need any help here? If so let us know!

@Thihup
Copy link
Collaborator

Thihup commented Oct 30, 2021

@mnriem What I'm seeing is that when testing Piranha Micro, the Shrinkwrap Maven resolver changes something in the filesystem (folder ~/.m2) in the same instant as Maven is doing something else. I actually don't know how to solve it.
The changes in the PR already improves time using mvn package, but mvn verify / mvn install still fail.

@mnriem
Copy link
Contributor

mnriem commented Oct 30, 2021

@arjantijms Any insight here? @Thihup do these changes require any changes to the current command line and or can they be committed regardless and for now only improve mvn package ?

@Thihup
Copy link
Collaborator

Thihup commented Oct 30, 2021

@mnriem The changes can be already committed. I didn't merged it yet because it didn't improve the mvn install, but we can merge it now and latter fix the mvn install 😉

Thihup added a commit to Thihup/piranha that referenced this issue Oct 30, 2021
Thihup added a commit to Thihup/piranha that referenced this issue Oct 30, 2021
This make sure that both projects have already built before running
the tests in parallel
Thihup added a commit to Thihup/piranha that referenced this issue Oct 30, 2021
@mnriem
Copy link
Contributor

mnriem commented Oct 30, 2021

I suggest we merge it and open a new issue to address the remaining work later.

Thihup added a commit that referenced this issue Oct 30, 2021
* Fixes issue #1948 - Use free ports instead of hardcoded ones in HttpServerTest

* Fixes issue #1948 - Add micro/server dependency in tests

This make sure that both projects have already built before running
the tests in parallel

* Fixes issue #1948 - Use free ports instead of the default (8080) in Piranha Micro tests
@mnriem mnriem added this to the 21.11.0 milestone Oct 31, 2021
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.

4 participants