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

8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories #6530

Closed
wants to merge 3 commits into from

Conversation

mlchung
Copy link
Member

@mlchung mlchung commented Nov 23, 2021

jdeps intends to report an error if there are multiple versions of the same class being parsed. module-info.class should be excluded from such detection.

This patch also fixes a data race in VersionHelper::set and also unwraps the ExecutionException when FutureTask of parsing the classes throws an exception to report MultiReleaseException properly.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

  • JDK-8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories
  • JDK-8277166: Data race in jdeps VersionHelper
  • JDK-8277123: jdeps does not report some exceptions correctly

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6530/head:pull/6530
$ git checkout pull/6530

Update a local copy of the PR:
$ git checkout pull/6530
$ git pull https://git.openjdk.java.net/jdk pull/6530/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6530

View PR using the GUI difftool:
$ git pr show -t 6530

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6530.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 23, 2021

👋 Welcome back mchung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@mlchung
Copy link
Member Author

mlchung commented Nov 23, 2021

/issue add JDK-8277564,JDK-8277166

@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 23, 2021
@openjdk
Copy link

openjdk bot commented Nov 23, 2021

@mlchung
Adding additional issue to issue list: 8277564: jdeps --print-module-deps fails with --multi-release.

Adding additional issue to issue list: 8277166: Data race in jdeps VersionHelper.

@openjdk
Copy link

openjdk bot commented Nov 23, 2021

@mlchung The following labels will be automatically applied to this pull request:

  • compiler
  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Nov 23, 2021
Copy link
Member

@JornVernee JornVernee left a comment

LGTM. Thanks for fixing.

@openjdk
Copy link

openjdk bot commented Nov 23, 2021

@mlchung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories
8277166: Data race in jdeps VersionHelper
8277123: jdeps does not report some exceptions correctly

Reviewed-by: jvernee, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 15 new commits pushed to the master branch:

  • 951247c: 8235876: Misleading warning message in java source-file mode
  • 663e33d: 8272728: javac ignores any -J option in @argfiles silently
  • 0384739: 8276665: ObjectInputStream.GetField.get(name, object) should throw ClassNotFoundException
  • cf7adae: 8275320: NMT should perform buffer overrun checks
  • 96e3607: 8275063: Implementation of Foreign Function & Memory API (Second incubator)
  • 17e68ca: 8277042: add test for 8276036 to compiler/codecache
  • 8a8bc29: 8277562: Remove dead method c1 If::swap_sux
  • d085c2b: 8273328: Compiler implementation for Pattern Matching for switch (Second Preview)
  • 6d73460: 8277399: ZGC: Move worker thread logging out of gc+phase=debug
  • 712b875: 8277397: ZGC: Add JFR event for temporary latency measurements
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/e8acac2aba30e8a172ca14509220ec06e833d4af...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 23, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 23, 2021

Webrevs

@mlchung
Copy link
Member Author

mlchung commented Nov 23, 2021

/label remove compiler

@openjdk openjdk bot removed the compiler compiler-dev@openjdk.org label Nov 23, 2021
@openjdk
Copy link

openjdk bot commented Nov 23, 2021

@mlchung
The compiler label was successfully removed.

@mlchung
Copy link
Member Author

mlchung commented Nov 23, 2021

/issue add JDK-8277123

@openjdk
Copy link

openjdk bot commented Nov 23, 2021

@mlchung
Adding additional issue to issue list: 8277123: jdeps does not report some exceptions correctly.

@mlchung
Copy link
Member Author

mlchung commented Nov 23, 2021

/issue remove JDK-8277564

@openjdk
Copy link

openjdk bot commented Nov 23, 2021

@mlchung
Removing additional issue from issue list: 8277564.

} else if (t instanceof Error) {
throw (Error)t;
} else {
throw new Error(e);
Copy link
Contributor

@AlanBateman AlanBateman Nov 24, 2021

Choose a reason for hiding this comment

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

A minor suggestion is that you could avoid the casts with:

Throwable cause = ...
if (cause instanceof RuntimeException e) {
throw e;
} else if (cause instanceof Error e) {
throw e;
} else {

Copy link
Member Author

@mlchung mlchung Nov 24, 2021

Choose a reason for hiding this comment

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

Fixed. Thanks for reminding.

@jaikiran
Copy link
Member

jaikiran commented Nov 25, 2021

Hello Mandy, one small issue - ClassFileReader, DependencyFinder and VersionHelper are misisng a copyright year update for this change.

@mlchung
Copy link
Member Author

mlchung commented Nov 26, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Nov 26, 2021

Going to push as commit 7e54d06.
Since your change was applied there have been 27 commits pushed to the master branch:

  • eb4d886: 8277504: Use String.stripTrailing instead of hand-crafted method in SwingUtilities2
  • 9879920: 8277825: Remove unused ReferenceProcessorPhaseTimes::_sub_phases_total_time_ms
  • f788834: 8277786: G1: Rename log2_card_region_per_heap_region used in G1CardSet
  • 3034ae8: 8277631: ZGC: CriticalMetaspaceAllocation asserts
  • f0136ec: 8275687: runtime/CommandLine/PrintTouchedMethods test shouldn't catch RuntimeException
  • 21e302a: 8270435: UT: MonitorUsedDeflationThresholdTest failed: did not find too_many string in output
  • a81e4fc: 8258117: jar tool sets the time stamp of module-info.class entries to the current time
  • 26472bd: 8277811: ProblemList vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001/TestDescription.java
  • b5841ba: 8277806: 4 tools/jar failures per platform after JDK-8272728
  • e785f69: 8276124: Provide snippet support for properties files
  • ... and 17 more: https://git.openjdk.java.net/jdk/compare/e8acac2aba30e8a172ca14509220ec06e833d4af...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 26, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels Nov 26, 2021
@openjdk openjdk bot removed the rfr Pull request is ready for review label Nov 26, 2021
@openjdk
Copy link

openjdk bot commented Nov 26, 2021

@mlchung Pushed as commit 7e54d06.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@mlchung
Copy link
Member Author

mlchung commented Nov 26, 2021

@jaikiran Sorry, I missed your comment about the copyright year update. I will update them in other jdeps fix.

@mlbridge
Copy link

mlbridge bot commented Nov 26, 2021

Mailing list message from Jaikiran Pai on core-libs-dev:

On 26/11/21 7:32 am, Mandy Chung wrote:

On Thu, 25 Nov 2021 02:22:20 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

Mandy Chung has updated the pull request incrementally with one additional commit since the last revision:

minor fix to avoid casting
Hello Mandy, one small issue - `ClassFileReader`, `DependencyFinder` and `VersionHelper` are misisng a copyright year update for this change.
@jaikiran Sorry, I missed your comment about the copyright year update. I will update them in other jdeps fix.

That sounds fine, Mandy. Thank you.

-Jaikiran

@mlchung mlchung deleted the jdeps-fix branch Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants