Skip to content

[MCOMPILER-205] fixes incremental compilation#3

Closed
monperrus wants to merge 3 commits intoapache:masterfrom
monperrus:patch-2
Closed

[MCOMPILER-205] fixes incremental compilation#3
monperrus wants to merge 3 commits intoapache:masterfrom
monperrus:patch-2

Conversation

@monperrus
Copy link
Copy Markdown

@monperrus monperrus commented Feb 27, 2018

Fixes https://issues.apache.org/jira/browse/MCOMPILER-205.

  • When useIncrementalCompilation=false, always recompile all source files.
  • When useIncrementalCompilation=true (default), only recompiles stale sources.

When useIncrementalCompilation=false, always recompile all source files.
if ( useIncrementalCompilation )
if ( !useIncrementalCompilation )
{
getLog().debug( "useIncrementalCompilation enabled" );
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You forgot to invert these log lines.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

good point! fixed.

@yeroc
Copy link
Copy Markdown

yeroc commented Mar 29, 2019

What needs to be done to move this forward?

@eolivelli
Copy link
Copy Markdown

eolivelli commented Mar 29, 2019

Hi
Thanks for your work.
did you run the integration tests suite?

mvn verify -Prun-its

Btw actually in 3.8.0 you just have to turn incremental compilation off in order to build only stale sources.
This behaviour is very error prone and it is to be used with caution.

With you fix it seems that you are dropping the part that is detecting stale files and inverting the meaning of the flag.
Can you explain more?

@monperrus
Copy link
Copy Markdown
Author

the fix does not drop anything, it indeed inverts the flag so that the behavior or the flag does what the meaning suggests, https://issues.apache.org/jira/browse/MCOMPILER-205

incrementalBuildHelperRequest = new IncrementalBuildHelperRequest().inputFiles( sources );

// CHECKSTYLE_OFF: LineLength
if ( ( compiler.getCompilerOutputStyle().equals( CompilerOutputStyle.ONE_OUTPUT_FILE_FOR_ALL_INPUT_FILES ) && !canUpdateTarget )
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It seems to me that here we are losing this test.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this simplification is on purpose for regular behavior and maintainability: we are not using incremental compilation, so we consider all sources.

the else branch is responsible for detecting stale changes.

@rfscholte
Copy link
Copy Markdown
Contributor

What needs to be done to move this forward?

Did you read the comments of https://issues.apache.org/jira/browse/MCOMPILER-205 ? So in general it works as designed.
The root cause is a missing feature of https://codehaus-plexus.github.io/plexus-compiler/ where you can define is a specific compiler implementation has incremental support. Once that is available we can start applying it to this plugin.

@yeroc
Copy link
Copy Markdown

yeroc commented Apr 2, 2019

@rfscholte Thanks for the update. There's quite a bit of conflicting information on the ticket so for an outsider it's difficult to discern what's correct and what isn't. That said, I scanned through the ticket again and couldn't find a reference to a missing feature in the compiler. Can you elaborate on that or post a link to the relevant ticket for that?

@rfscholte
Copy link
Copy Markdown
Contributor

@yeroc nobody created an issue for it yet, but you might conclude it based on some comments. Both this PR and MCOMPILER-205 have a huge amount of comments, which makes it kind of hard to see the whole picture.
Let's start by closing this PR, I won't accept it since reversing the logic is not the solution.

https://stackoverflow.com/questions/16963012/maven-compiler-recompile-all-files-instead-modified/49700942#49700942 is the best summary (and the accepted answer in incorrect)

@rfscholte rfscholte closed this Apr 2, 2019
@cowwoc
Copy link
Copy Markdown

cowwoc commented Apr 2, 2019

Who will create the new issue? (I don't have a good enough understanding of this topic so I can't do it)

Please link the new issue to this one so we can follow. Thanks!

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.

6 participants