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.

Sign up for GitHub

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

Jump to bottom

Enabled java9+ compatibility #91

Merged
merged 1 commit into from Aug 25, 2020
Merged

Enabled java9+ compatibility #91

merged 1 commit into from Aug 25, 2020

Conversation

Finomosec
Copy link
Contributor

  • SignatureChecker: upgraded from ASM5 to ASM7
  • ClassFileVisitor: added ability to process Paths (nedded for "jrt:/modules")
  • SignatureBuilder: added java version switch (<=8 / >=9) and implemented signature creation for java 9+

@Finomosec
Enabled java9+ compatibility
13b1456 
- SignatureChecker: upgraded from ASM5 to ASM7
- ClassFileVisitor: added ability to process Paths (nedded for "jrt:/modules")
- SignatureBuilder: added java version switch (<=8 / >=9) and implemented signature creation for java 9+
@Finomosec Finomosec mentioned this pull request Aug 17, 2020
Closed
@olamy olamy added the dependencies Pull requests that update a dependency file label Aug 19, 2020
@olamy
Copy link
Member

olamy commented Aug 19, 2020

@Finomosec thanks for the pr. There is build failure with travix. Can you reproduce or any idea on it?
Thanks

@olamy olamy merged commit 6dec113 into mojohaus:master Aug 25, 2020
Marcono1234
Marcono1234 reviewed Nov 4, 2020
View reviewed changes
animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/ClassFileVisitor.java
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
if (file.getFileName().toString().endsWith(".class")) {
process(file.toString(), Files.newInputStream(file));
Copy link
Contributor

Choose a reason for hiding this comment

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

The InputStream created by Files.newInputStream(...) has to be closed again because it looks like the process(...) method does not do that.

@Marcono1234
Copy link
Contributor

It appears the order in which Files.walkFileTree(...) processes files within a directory is not defined. This might be problematic because enclosing classes have to be processed before nested classes. Otherwise a nested class is processed and forbidden API usage is detected despite the enclosing class being annotated.

Maybe a solution to this would be to create a SortedSet in FileVisitor.preVisitDirectory(...) to which all file paths of that directory are added and then only in FileVisitor.postVisitDirectory(...) process the files.

@Marcono1234 Marcono1234 mentioned this pull request Nov 4, 2020
Closed
@timja
Copy link

timja commented Dec 13, 2020

@olamy do you plan to release this? ❤️

This was referenced Dec 21, 2020
Closed
Merged
olamy pushed a commit that referenced this pull request Dec 23, 2020
@bradcupit
Fix "NestMember requires ASM7" (#135)
3976979 
Fixes #134

I saw the above error when building a work project that uses animal-sniffer. That work project uses JDK 11 but compiles some modules for JDK 8 and uses animal-sniffer to verify correct JDK 8 api usage. I grepped the animal-sniffer code and found one remaining occurrence of ASM5. After Replacing it with ASM7, and doing a local `mvn clean install` on animal-sniffer, my work project built correctly.

Related to this PR: #91
@cowwoc
Copy link

cowwoc commented Dec 31, 2020

It appears the order in which Files.walkFileTree(...) processes files within a directory is not defined. This might be problematic because enclosing classes have to be processed before nested classes. Otherwise a nested class is processed and forbidden API usage is detected despite the enclosing class being annotated.

Maybe a solution to this would be to create a SortedSet in FileVisitor.preVisitDirectory(...) to which all file paths of that directory are added and then only in FileVisitor.postVisitDirectory(...) process the files.

@Finomosec any chance you could address these concerns?

@cowwoc cowwoc mentioned this pull request Dec 31, 2020
Closed
@Finomosec
Copy link
Contributor Author

Finomosec commented Jan 2, 2021 •
edited

@Finomosec any chance you could address these concerns?

Yes, i will look into this.
How can i robustly identify nested classes from their file name?
Do they always contain a $ in their name?

@Marcono1234
Copy link
Contributor

Do they always contain a $ in their name?

It should be safe to assume this (at least for Java), see JLS § 13.1 which says:

The binary name of a member type (§8.5, §9.5) consists of the binary name of its immediately enclosing type, followed by $, followed by the simple name of the member.

So the solution proposed above should work; it might not even require a custom comparator when using the SortedSet. (Feel free to implement the solution as proposed; or please let me know if I should try implementing the solution).

Could you then please also address #91 (comment)?

@Finomosec
Copy link
Contributor Author

Finomosec commented Jan 19, 2021 •
edited

Here is the new PR to fix this + I also fixed org.codehaus.mojo.animal_sniffer.Main to support Java 7+ and URIs like jrt:/modules: #139

@cowwoc Can you confirm the fix worked as expected?

@Finomosec Finomosec mentioned this pull request Jan 19, 2021
Merged
@cowwoc
Copy link

cowwoc commented Jan 24, 2021

@Marcono1234 can you please verify @Finomosec's new PR since you raised these concerns?

@Marcono1234
Copy link
Contributor

@cowwoc, I think these changes should work. I have added some minor review comments.

@kwin kwin mentioned this pull request Feb 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Marcono1234 Marcono1234 Marcono1234 left review comments

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Finomosec @olamy @Marcono1234 @timja @cowwoc