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

Updated ASM8 to ASM9 #12487

Merged
merged 1 commit into from
Dec 3, 2021
Merged

Updated ASM8 to ASM9 #12487

merged 1 commit into from
Dec 3, 2021

Conversation

mstahv
Copy link
Member

@mstahv mstahv commented Nov 30, 2021

No description provided.

@mstahv mstahv mentioned this pull request Nov 30, 2021
Merged
8 tasks
@github-actions
Copy link

github-actions bot commented Nov 30, 2021 •
edited
Loading

Unit Test Results

   717 files     717 suites   17m 22s ⏱️
5 615 tests 5 566 ✔️ 49 💤 0 ❌
5 628 runs  5 579 ✔️ 49 💤 0 ❌

Results for commit 31c5365.

♻️ This comment has been updated with latest results.

knoobie
knoobie reviewed Nov 30, 2021
View reviewed changes
...ver/src/main/java/com/vaadin/flow/server/frontend/scanner/FrontendAnnotatedClassVisitor.java
@@ -60,7 +60,7 @@
* The annotation class name to visit
*/
FrontendAnnotatedClassVisitor(ClassFinder finder, String annotationName) {
super(Opcodes.ASM8);
Copy link
Contributor

Choose a reason for hiding this comment

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

This (https://gitlab.ow2.org/asm/asm/-/issues/317854) could probably explain why the OpCodes has to be updated for newer java releases to work properly. Comment by Remi Forax:

I will close this issue as work as intended even if you may think it's surprising, currently ASM ClassReader support Java 11 (and even Java 12) so it recognizes Java 11 new attributes (here NestMembers). But here you are using a ClassVisitor which is configured to only recognize the ASM6 API, thus it fails at runtime with an UnsupportedOperation exception.

@Artur-
Copy link
Member

Artur- commented Dec 1, 2021

Can we merge this? Why is it a draft?

@mstahv
Copy link
Member Author

mstahv commented Dec 3, 2021

I can't test that it actually works. I cannot reproduce the issue. Maybe going in blind is sometimes the option 😎

@mstahv mstahv marked this pull request as ready for review December 3, 2021 07:10
@mstahv
Copy link
Member Author

mstahv commented Dec 3, 2021

I can't test that it actually works. I cannot reproduce the issue. Maybe going in blind is sometimes the option 😎 @vaadin/flow Up to you to make the call.

@caalador caalador merged commit c8e8054 into master Dec 3, 2021
vaadin-bot pushed a commit that referenced this pull request Dec 3, 2021
@mstahv @vaadin-bot
fix: Updated ASM8 to ASM9 (#12487)
a967881 
Update ASM to support Java 17
@mstahv
Copy link
Member Author

mstahv commented Dec 8, 2021

@caalador Can you CP this for Vaadin 14 series as well?

caalador pushed a commit that referenced this pull request Dec 8, 2021
@mstahv @caalador
fix: Updated ASM8 to ASM9 (#12487)
ae4c829 
Update ASM to support Java 17
caalador added a commit that referenced this pull request Dec 8, 2021
@caalador @mstahv
fix: Updated ASM8 to ASM9 (#12487) (#12534)
04a66d5 
Update ASM to support Java 17

Co-authored-by: Matti Tahvonen <[email protected]>
vaadin-bot pushed a commit that referenced this pull request Dec 8, 2021
@caalador @mstahv @vaadin-bot
fix: Updated ASM8 to ASM9 (#12487) (#12534)
39ac0a0 
Update ASM to support Java 17

Co-authored-by: Matti Tahvonen <[email protected]>
vaadin-bot added a commit that referenced this pull request Dec 8, 2021
@vaadin-bot @caalador @mstahv
fix: Updated ASM8 to ASM9 (#12487) (#12534) (#12536)
bfc6125 
Update ASM to support Java 17

Co-authored-by: Matti Tahvonen <[email protected]>

Co-authored-by: caalador <[email protected]>
Co-authored-by: Matti Tahvonen <[email protected]>
@mstahv
Copy link
Member Author

mstahv commented Dec 8, 2021

@caalador For some reason the fix has not been picked to 9.0 (aka Vaadin 22) branch. https://github.com/vaadin/flow/blob/9.0/flow-server/src/main/java/com/vaadin/flow/server/frontend/scanner/FrontendAnnotatedClassVisitor.java#L63

caalador pushed a commit that referenced this pull request Dec 8, 2021
@vaadin-bot @mstahv
fix: Updated ASM8 to ASM9 (#12487) (#12504)
ba50019 
Update ASM to support Java 17

Co-authored-by: Matti Tahvonen <[email protected]>
@caalador
Copy link
Contributor

caalador commented Dec 8, 2021

Auto merge had not merged for some reason.

@caalador caalador deleted the feature/asm9 branch October 12, 2022 10:43
@vaadin-bot
Copy link
Collaborator

Hi @mstahv and @caalador, when i performed cherry-pick to this commit to 2.8, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick c8e8054
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

git commit --allow-empty

Otherwise, please use 'git cherry-pick --skip'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knoobie knoobie knoobie left review comments

@caalador caalador caalador approved these changes

@Artur- Artur- Artur- approved these changes

Assignees
No one assigned
Labels
cherry-picked-9.0 need to pick manually 2.8 +0.0.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mstahv @Artur- @caalador @vaadin-bot @knoobie