FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[MCOMPILER-525] Incorrect detection of dependency change by jorsol · Pull Request #172 · apache/maven-compiler-plugin · GitHub

  1. apache
  2. maven-compiler-plugin

Repository navigation

[MCOMPILER-525] Incorrect detection of dependency change - #172

Merged
gnodet merged 3 commits into
apache:masterfrom
jorsol:MCOMPILER-525
Feb 13, 2023
ConversationCommits (3)ChecksFiles changed
Merged

[MCOMPILER-525] Incorrect detection of dependency change#172
gnodet merged 3 commits into
apache:masterfrom
jorsol:MCOMPILER-525

Conversation

jorsol commented Jan 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This fix a bug with the incremental compilation related to the incorrect detection of the project classes as a dependency.

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MCOMPILER-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MCOMPILER-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

olamy commented Jan 29, 2023

Copy link
Copy Markdown
Member

@jorsol thanks for your PR. is your issue fixed if you try this PR https://github.com/apache/maven-compiler-plugin/pull/172/files#r1090053416?
because I'm a bit scared of the performance impact of some changes here

jorsol commented Jan 30, 2023

Copy link
Copy Markdown
Contributor Author

Hi @olamy, the issue is not fixed with the PR #163.

I haven't done any performance testing, but I'm sure this will be more performant than the current code, yet I could check and improve the code further if needed.

jorsol force-pushed the MCOMPILER-525 branch 4 times, most recently from b1c7189 to 0dac7fd Compare January 31, 2023 10:49

This comment was marked as resolved.

Sign in to view
jorsol force-pushed the MCOMPILER-525 branch 3 times, most recently from 4d83128 to 9e1a2bd Compare February 1, 2023 15:54
slawekjaranowski reviewed Feb 1, 2023
View reviewed changes
Comment thread src/it/MCOMPILER-525/pom.xml Outdated
jorsol force-pushed the MCOMPILER-525 branch 2 times, most recently from e9052ad to 005ae0f Compare February 2, 2023 11:21

jorsol commented Feb 2, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@slawekjaranowski @olamy I think I have addressed all the comments, even I end up basically fixing the issue MCOMPILER-381 too.

There are a lot of other improvements that can be made, refactor legacy code, and more, but for now I don't want to keep touching and touching this (every time I look I end up making some small refactor), this should be a good improvement so far, as of performance concerns this should be at least equally performant than the previous code.

jorsol force-pushed the MCOMPILER-525 branch 3 times, most recently from 40c3cf2 to f6db9fb Compare February 2, 2023 15:13
olamy requested changes Feb 3, 2023
View reviewed changes
Comment thread src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java Outdated
Comment thread src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java Outdated

gnodet commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

@jorsol I have a hard time understanding the exact change that fixes the problem. Would it be possible to have a first commit that refactors the code using the nio api and other improvements, and another commit with the actual fix for the problem ?

Note that we could also embeds the trimmed down version of the IncrementalBuildHelper.

jorsol commented Feb 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I have a hard time understanding the exact change that fixes the problem.

@gnodet sure, the fix for this issue is trivial, so I undo all the refactoring and only provide the actual fix.

gnodet requested a review from olamy February 10, 2023 15:55
gnodet changed the title [MCOMPILER-525] - Incremental recompile incorrect detection of dependency change [MCOMPILER-525] Incorrect detection of dependency change Feb 10, 2023
[MCOMPILER-525] - Incremental recompile incorrect detection of depend…
e6c9bc4 
…ency change

Signed-off-by: Jorge Solórzano <jorsol@gmail.com>
gnodet added this to the 3.11.0 milestone Feb 13, 2023
gnodet merged commit f4a8a54 into apache:master Feb 13, 2023
jorsol deleted the MCOMPILER-525 branch February 13, 2023 10:12

jorsol commented Feb 13, 2023

Copy link
Copy Markdown
Contributor Author

Thanks @gnodet , I will open the refactor PR for MCOMPILER-381 that was previously done here ASAP and then later I could check if I could use the IncrementalBuildHelper mentioned if time allows me.

slawekjaranowski added the bug Something isn't working label Feb 28, 2023
sos-jira-migration-bot Bot mentioned this pull request Feb 27, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

slawekjaranowski slawekjaranowski left review comments
gnodet gnodet approved these changes
olamy Awaiting requested review from olamy
+1 more reviewer
pzygielo pzygielo left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

3.11.0

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL