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

Support for transformation from jakartaee8 to jakartaee10 applications #5847

Conversation

breakponchito
Copy link
Contributor

@breakponchito breakponchito commented Apr 19, 2023

This PR adds support to transform applications from Jakarta EE 8 to Jakarta 10. When you have an older application and you want to transform to the Jakarta EE 10, then you can use the new option from the new project wizard to make the transformation. This is useful for developers that want to migrate older applications to the new Jakarta 10.

Case 1: file to a different folder

filetofolder.mp4

Case 2: folder to a different folder

foldertofolder.mp4

Case 3: file on the same location (override)

filetosamelocation.mp4

Case 4: folder on the same location (override)

folderonsamelocation.mp4

^Add meaningful description above

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

  • are all your own work, and you have the right to contribute them.
  • are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

@mbien mbien added the Java EE/Jakarta EE [ci] enable enterprise job label Apr 19, 2023
@mbien mbien added this to the NB19 milestone Apr 19, 2023
@breakponchito
Copy link
Contributor Author

@mbien Can you help to start again the validations?

@mbien mbien added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Apr 24, 2023
@breakponchito
Copy link
Contributor Author

@mbien please send another test for the validations

@mbien
Copy link
Member

mbien commented Apr 24, 2023

@breakponchito you can run commit validation locally, build it first then simply run ant commit-validation

going to start it later since we have 7 test runs in parallel running atm and I don't want to DoS the shared apache infrastructure.

@breakponchito
Copy link
Contributor Author

@mbien Do you know what is causing the following error on the pipeline?
[junit] Testcase: testInvisibleModules(org.netbeans.core.validation.ValidateModulesTest): FAILED [junit] Some regular modules (that no one depends on) neither AutoUpdate-Show-In-Client=true nor AutoUpdate-Essential-Module=true (thus unreachable through Plugin Manager) [junit] org.netbeans.modules.jakarta.transformer Log: [junit] Starting test shuttingDown[org.netbeans.core.validation.ValidateModulesTest] [junit] [org.netbeans.core.NbLifecycleManager] THREAD: Test Watch Dog: shuttingDown[org.netbeans.core.validation.ValidateModulesTest] MSG: Initiating exit with status 0 [junit] [org.netbeans.core.NbLifecycleManager] THREAD: Test Watch Dog: shuttingDown[org.netbeans.core.validation.ValidateModulesTest] MSG: blockForExit, new org.netbeans.core.NbLifecycleManager$1@0[Count = 1] [junit] [org.netbeans.core.NbLifecycleManager] THREAD: Test Watch Dog: shuttingDown[org.netbeans.core.validation.ValidateModulesTest] MSG: NbLifeExit(0, 0, null, org.netbeans.core.NbLifecycleManager$1@0[Count = 1]) = org.netbeans.core.NbLifeExit@1

I tried to add both properties but this said that both are not reachable for the Plugin Manager, and this is happening for the following JDK's: 11, 17 and 21. Please let me know if you have any suggestion

@neilcsmith-net
Copy link
Member

@breakponchito it's complaining that neither of those properties are true. Probably need to make this autoload, and then make it a dependency or recommendation of j2ee.kit. If this requires jakartaee10 modules to be enabled, then probably should follow similar to neilcsmith-net@5d3aa15

Now we don't need to special case these for JDK 8, should consider a different way to enable these too, but I guess should be done together.

@breakponchito
Copy link
Contributor Author

breakponchito commented May 11, 2023

@mbien @neilcsmith-net Do you know how to debug unit tests from local environment? or Do you have some references to see how to debug the commit-validations tests?

@breakponchito
Copy link
Contributor Author

@mbien @neilcsmith-net
to fix my local issue for the commit-validation it was needed to edit the following file:

  • php/php.editor/manifest.mf
  • module affected: org.netbeans.modules.php.editor
    by setting the following variables as true:
    AutoUpdate-Show-In-Client: true AutoUpdate-Essential-Module: true

@breakponchito
Copy link
Contributor Author

@mbien I merged last changes from master branch, could you help me to send the pipeline validations?

Copy link
Contributor

@OndroMih OndroMih left a comment

Choose a reason for hiding this comment

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

Hi, @breakponchito , thank you for this contribution. I'm looking forward to start using it when it's released!

@breakponchito
Copy link
Contributor Author

I added some commits, could you help me to send the pipeline validations?

@mbien mbien added the ci:all-tests [ci] enable all tests label Jun 23, 2023
@apache apache locked and limited conversation to collaborators Jun 23, 2023
@apache apache unlocked this conversation Jun 23, 2023
@breakponchito
Copy link
Contributor Author

Hi, @breakponchito , thank you for this contribution. I'm looking forward to start using it when it's released!

@OndroMih @jGauravGupta please let me know if any other comment about this PR. I'm waiting for approvals

@jGauravGupta
Copy link
Contributor

Hi @breakponchito,

I have reviewed the code and it looks good to me. There are two improvements that I suggest:

  • Bind an event to the source and target text fields: Currently, there is a warning message displayed when a path is added directly in the text field. To improve this, you can bind an event to the text fields so that the warning message is automatically removed when a path is added directly into the text fields instead of using the browse button.

  • Remove the package goal from the execution command: It seems that the execution command is invoking the maven-war-plugin unnecessarily. To optimize the command, you can remove the package goal, as it may not be required for your specific use case. This will help streamline the execution process and remove any unnecessary steps.

@breakponchito
Copy link
Contributor Author

Hi @breakponchito,

I have reviewed the code and it looks good to me. There are two improvements that I suggest:

  • Bind an event to the source and target text fields: Currently, there is a warning message displayed when a path is added directly in the text field. To improve this, you can bind an event to the text fields so that the warning message is automatically removed when a path is added directly into the text fields instead of using the browse button.
  • Remove the package goal from the execution command: It seems that the execution command is invoking the maven-war-plugin unnecessarily. To optimize the command, you can remove the package goal, as it may not be required for your specific use case. This will help streamline the execution process and remove any unnecessary steps.

already done, please review

@breakponchito
Copy link
Contributor Author

@mbien I added a new commit, Could you help me to send the pipeline validations?

@mbien
Copy link
Member

mbien commented Jul 12, 2023

@breakponchito I am going to add a commit - I hope this is ok. I wanted to add review comments but it is faster to code it right away since some of it is UI.

  • made the wizard panel a bit more compact and layout gaps more consistent
  • removed a file chooser component which was stored in the form but not used
  • the text fields should validate on input change rather than action performed (which is pressing return). Otherwise the user can't simply type /tmp and press finish. For that we can use a DocumentListener
  • code cleanup: overrides, generics, arm blocks, compiler warnings etc
  • fixed a NPE which happened while closing the wizard.
java.lang.NullPointerException: Cannot invoke "org.openide.WizardDescriptor.putProperty(String, Object)" because "this.wizardDescriptor" is null
	at org.netbeans.modules.fish.payara.jakarta.transformer.TransformerWizardIterator.uninitialize(TransformerWizardIterator.java:446)
	at org.openide.loaders.TemplateWizard$InstantiatingIteratorBridge.uninitialize(TemplateWizard.java:1076)
	at org.openide.loaders.TemplateWizardIterImpl.uninitialize(TemplateWizardIterImpl.java:215)
	at org.openide.loaders.TemplateWizardIteratorWrapper.uninitialize(TemplateWizardIteratorWrapper.java:131)
	at org.openide.WizardDescriptor.callUninitialize(WizardDescriptor.java:1541)
	at org.openide.WizardDescriptor.resetWizardOpen(WizardDescriptor.java:1383)
	at org.openide.WizardDescriptor.resetWizard(WizardDescriptor.java:1362)
[catch] at org.openide.WizardDescriptor.setValueOpen(WizardDescriptor.java:1346)

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) and removed ci:all-tests [ci] enable all tests labels Jul 12, 2023
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

I am not 100% convinced that a new project wizard is the best vehicle to integrate this kind of feature which is essentially code refactoring.
It is working but it feels a bit like it is sidelining all other IDE features, e.g diffs before changes are applied etc.
I am glad that it is here though since it certainly could be useful for some.

@breakponchito
Copy link
Contributor Author

@breakponchito I am going to add a commit - I hope this is ok. I wanted to add review comments but it is faster to code it right away since some of it is UI.

  • made the wizard panel a bit more compact and layout gaps more consistent
  • removed a file chooser component which was stored in the form but not used
  • the text fields should validate on input change rather than action performed (which is pressing return). Otherwise the user can't simply type /tmp and press finish. For that we can use a DocumentListener
  • code cleanup: overrides, generics, arm blocks, compiler warnings etc
  • fixed a NPE which happened while closing the wizard.
java.lang.NullPointerException: Cannot invoke "org.openide.WizardDescriptor.putProperty(String, Object)" because "this.wizardDescriptor" is null
	at org.netbeans.modules.fish.payara.jakarta.transformer.TransformerWizardIterator.uninitialize(TransformerWizardIterator.java:446)
	at org.openide.loaders.TemplateWizard$InstantiatingIteratorBridge.uninitialize(TemplateWizard.java:1076)
	at org.openide.loaders.TemplateWizardIterImpl.uninitialize(TemplateWizardIterImpl.java:215)
	at org.openide.loaders.TemplateWizardIteratorWrapper.uninitialize(TemplateWizardIteratorWrapper.java:131)
	at org.openide.WizardDescriptor.callUninitialize(WizardDescriptor.java:1541)
	at org.openide.WizardDescriptor.resetWizardOpen(WizardDescriptor.java:1383)
	at org.openide.WizardDescriptor.resetWizard(WizardDescriptor.java:1362)
[catch] at org.openide.WizardDescriptor.setValueOpen(WizardDescriptor.java:1346)

thank you for your fixes

@breakponchito
Copy link
Contributor Author

I am not 100% convinced that a new project wizard is the best vehicle to integrate this kind of feature which is essentially code refactoring. It is working but it feels a bit like it is sidelining all other IDE features, e.g diffs before changes are applied etc. I am glad that it is here though since it certainly could be useful for some.

Regarding the place of this, at the beginning we though to add the functionality to the pop up menus when selecting folder or file, but after reviewing we though to add it on the project wizard. If you have an idea where to add it to be a better place, please let me know and I can change it

@OndroMih
Copy link
Contributor

I am not 100% convinced that a new project wizard is the best vehicle to integrate this kind of feature which is essentially code refactoring. It is working but it feels a bit like it is sidelining all other IDE features, e.g diffs before changes are applied etc. I am glad that it is here though since it certainly could be useful for some.

I agree that this solution is not ideal and feels half-baked. But I understand it was already a lot of effort to build this solution and it's better than nothing.

In the future, I'd like to have this as an Inspect & Transform rule in the Refactor menu, here:
image

This would allow to open a Java EE project and apply refactoring in place, directly on the project, including preview of changes if possible.

If we decide to keep the approach of a new project, I would at least expect that the generated project is opened in Netbeans after it's generated. That's what generally New project wizards do - they create a project in Netbeans, not just on the filesystem.

@mbien
Copy link
Member

mbien commented Jul 13, 2023

If you have an idea where to add it to be a better place, please let me know and I can change it

There are several places where code transformation could be integrated. It could be triggered by an in-editor code hint/inspection.
Clicking at the light bulb (or running the code inspection manually) could: suggest to transform the current file or the whole project and should ideally result in a refactoring preview/diff.

But this could be still extended later if the feature remains popular. Not everything has to be solved by the same PR.

@mbien mbien added ci:all-tests [ci] enable all tests and removed ci:all-tests [ci] enable all tests labels Jul 13, 2023
@matthiasblaesing
Copy link
Contributor

The suggestion from @OndroMih sound sane, it would also imply, that not all suggested modes would work (at least I'm under the impression, that the hints engine always works in-place).

Locating this in the "New project" dialog is odd and the last place I would look for modifying an existing project.

If we want to get this in fast and now, then I suggest to make this an entry in the "Tools" menu. From a UX perspective though, I would prefer integration through the hint interface, as this nicely aligns with general cleanup and platform updates.

@mbien
Copy link
Member

mbien commented Jul 14, 2023

Locating this in the "New project" dialog is odd and the last place I would look for modifying an existing project.

If we want to get this in fast and now, then I suggest to make this an entry in the "Tools" menu. From a UX perspective though, I would prefer integration through the hint interface, as this nicely aligns with general cleanup and platform updates.

@matthiasblaesing Honestly, tools menu isn't much better IMO - its also not a place where I look for refactoring tasks. The implementation right now is separate from everything and not intrusive, I would be ok with integrating it as experimental feature, it seems like there is interest to properly integrate it at some point in future. (But my initial reaction was similar to yours)

The module has no public API (I removed that export), so we could remove it in future again if there is no interest to improve it. It could be extracted to a plugin (plugin portal) for example in that case.

This could be even delivered as maven hint, since it could directly annotate the dependency in the pom, replace it, then run the transformation.

We could of course also move this to NB 20, but the feature does work - maybe someone will find it useful in its early stage.

@breakponchito
Copy link
Contributor Author

I'm agree with you guys that this tool can be improved, as @mbien said probably we can accept, merge and go with this as an experimental feature, then I will work to improve it for the next release. What do you think?

Co-authored-by: Michael Bien <mbien42@gmail.com>
@mbien
Copy link
Member

mbien commented Jul 16, 2023

I have a squashed and rebased version of this PR here: https://github.com/mbien/netbeans/tree/FISH-6771-support-transformation-jakartaee8-to-jakartaee10

(I removed the FISH-6771 from the msg since I believe that this is an internal issue tracker)

If everyone is ok with this I can force push into this PR and merge on Sunday evening. (we don't use gh ui for squashing since it caused issues before with noreply mails in author headers etc)

@juneau001
Copy link
Contributor

I'd be happy to see this in NetBeans 19 and improved in a future release, as needed. Nice work! This will be very nice indeed for helping to further Jakarta EE 10 adoption.

@mbien mbien force-pushed the FISH-6771-support-transformation-jakartaee8-to-jakartaee10 branch from 68f7fe4 to 8af5722 Compare July 16, 2023 14:09
@mbien
Copy link
Member

mbien commented Jul 16, 2023

@breakponchito congrats to your first contribution and thanks for your patience -> merging

@mbien mbien merged commit d848cb3 into apache:master Jul 16, 2023
@breakponchito
Copy link
Contributor Author

@mbien @jGauravGupta @OndroMih @matthiasblaesing thank you guys for your help an patience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Java EE/Jakarta EE [ci] enable enterprise job Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants