-
Notifications
You must be signed in to change notification settings - Fork 0
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
9727 jakarta servlet support #3
9727 jakarta servlet support #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll give this a try, and post the javadoc so we can get some more eyes on it.
Can you split the readme fixes into their own PR directly to the upstream repo, so we can land them on their own?
Thanks for your feedback. I created a new PR for the readme stuff. |
I have notice that during the file copy and replace operation the encoding will not be set.
Does it make sense to open a PR for that? |
Thanks for the heads up @rdeangelis83 - it would be great if you sent a PR to help with that. I'm hoping for now at least that we do not duplicate any files within the codebase, only in the output, so that we can use the same code to generate two jars (javax vs jakarta). Eventually there may come a time when we actually fork the files, so that the jakarta code can take advantage of new features, or deal with missing/removed methods. Does that make sense? |
Hi @niloc132 , @sparsick I have taken a look in which packages we have a reference to some
We could now move everything in these packages into a corresponding jakarta package:
And also change the package name in those classes. The problem that I see currently with this approach is that the classes from these packages will also be used also somewhere outside. This means for those classes the javadoc would be still wrong. One example: So, If we don't move also these classes to a corresponding jakarta package, the javadoc will still be wrong. And this will also mean that we have to check where these classes will be referenced and so on ... Comments/Opinions? |
@rdeangelis83 I think @niloc132 mentions your idea here. I would adjust my PR next week, to do excatily this. |
Hi @sparsick, But do you have already an idea how exactly you will do it?
|
My first ideas:
I'd like to write some Java code that recognize which classes were matched and copy this list of files to the corresponding jakarta package.
Like above. I know which packages were matched, so I can search for imports and move the matches classes to new jakarta packages. What do you think? Do you have another approach? |
If we move only single classes from a package into a jakarta specific one, we could run in the issue that something is only package visible and in additional you can not search for imports because classes in the same package don't need to be imported and you have to add the imports . But on the other hand, we have a much smaller list of classes, so that maybe the problem that I have described will not be a problem anymore. So somehow I still think that moving the entire package would be better. |
I haven't tried this, but I think we can probably take a simpler approach:
My thinking here is that we're going to have a few classes which are referenced by servlet classes, but hopefully not a huge list of class which depend on servlet classes. Additionally, the list won't be growing (or at least not fast), as this code is fairly stable, so a manual approach seems like it should be okay. Hopefully, these classes will follow some kind of helpful pattern, so we can use a broad brush to fix this. As before, I'm not terribly concerned about "breaking" changes where an application is already deliberately breaking APIs. Especially not for a first cut. Here's my quick attempt at the above: Direct references:
First degree indirect references:
Second degree indirect references - All are via SerializabilityUtil, we could in theory clone the RPCServletUtils.CHARSET_UTF8 field so that this is no longer an issue
Technically indirectly referenced, but only from javadoc, should probably edit this too?:
If we make a small change to SerializabilityUtil to get its own reference to CHARSET_UTF8, then we have 9 classes that need relocation by my count, with 6 of those that need import changes. I'm inclined to say that a static list in the build file (which would then be checked by the compiler) would do the job effectively. I'm not sure we could do better manually even by forking all of these files. |
Hi @niloc132 Yeah that sounds feasible. One last question: What about |
Hi @niloc132, I also like your simple approach. I will try your approach how it works. I would also refactor the CHARSET_UTF8 issue or it is better to open an own PR for that? |
@rdeangelis83 I think if it is worth doing, it is worth doing both server jars, rather just one (esp since I left out the requestfactory server jars from the list above deliberately... and there are a few more dependencies that way). The big hiccup there is that I don't see a reasonable way to migrate javax.validation without also rewriting all of the client code that uses validation, and I think that has to stay out of scope for this. @sparsick I'll go ahead and land that right away - I think we should actually go ahead and remove the field entirely, and use StandardCharsets instead. |
Fix has been reviewed and merged upstream, and I've merged that into my branch. |
Hi @niloc132, I implemented the movement to an own jakarta package. The implemented list of files is longer than yours because of the dependency to other classes in the same package. If this is fine, I will go ahead with the javadoc stuff. Other things that came me to my mind found during the doing:
|
Hi @sparsick, I don't see any propblem to move also the GWTServletBase class. Regarding the the question whether we need an own jar file: @niloc132 your opinion? |
Thanks, @rdeangelis83, for your feedback. I moved GWTServletBase and its used classes in an own package. Maybe we should stay with two jars, because I have to have replaced import statements on the root level and when everything is in one jar with have a decision problem which class should be chosen. Next step is javadoc, but also here what about the import statement outside the jakarta packages? |
Ohh yes of course: That was already a point that I mentioned on my first comment :) So let's move forward with two jars. |
Link to mvn README.txt from the main repo README.md
I don't mean to suggest that these are good ideas, just that they are ideas to consider. |
@niloc132 @rdeangelis83 I went ahead with the javadoc stuff. I create a temp src folder to prepare the jakarta related stuff for the javadoc, so the jacadoc for all classes can put together. @niloc132 the current state would work for my project to go ahead with the Spring Boot 3 upgrade. Do I catch every open issue to finish this PR? |
Hi @sparsick could you add the encoding for the copy operation of the jakarta-src folder.
I will have a look to your changes and see if I find something that we forgot ... |
@rdeangelis83 done! Thank you |
Dropped the frames requirement, and enabled index and tree pages.
Disables tests that fail on Windows because of filesystem specifics, fixes one test. Co-authored-by: Zbynek Konecny <[email protected]>
@rdeangelis83 did you have time to take a look at my changes? |
Looks like stylecheck fails, can you see about excluding the new generated directory from that check? |
@niloc132 it seems that my local build run does not match all cases. |
I left the project, where this patch is needed. Therefore, I have no possibilities to test the patch. Maybe @lofidewanto can help here. |
@niloc132 I reviewed your patch. Your generated servlet-jakarta.jar is smaller than my generated servlet-jakarta.jar because my jar has included java classes. Also your jar has no core.server.jakarta package, but this is needed because of GwtServletBase.java |
Thanks, removing GwtServletBase was an oversight that I'll correct. I'll also investigate ensuring that generated sources are included in the jar (or at least in the source jar). With that said, I'm going to merge this and my own changes into my branch, and update the ticket discussing this. I'll spend a little free time on this to make sure all the work in progress is accessible, but without someone to keep it moving, it isn't likely to make the next release. |
I thought it might be helpful to try out the build mentioned at #3 (comment) by @niloc132 . In my source code I've changed all the Java EE 8 (I was using Payara 5)
The invoking ant task looks like:
The classpathref Did I miss something important or is this an issue to be fixed? |
@dbbernstein it looks like you might have downloaded the gwt-dev jar directly from maven, but tried to use it in ant without any of its other dependencies? If you use the "full" jars (which are packaged as part of the GWT SDK release zip), then ant/etc can use them without any dependencies, but when using a tool that can speak to a maven repository, we assume that it can also fetch the required dependencies. Here's a build from a recent commit I pushed, produced by github actions - if you script down, you'll find a gwt-java11 zip download, which should be the full sdk (it is slightly bigger than the java8 build because javadoc includes a few more resources), which includes the "dependencies included" gwt dev you'll be looking for: https://github.com/niloc132/gwt/actions/runs/5315057779 |
After a bit of struggle i finally managed to directly access the repo from company network, for some reason our nexus does not like to proxy it as a remote repo. I updated the dependencies in our utils project(s) (by replacing any chance to get this replaced too?
I'm looking forward to this. As soon as you are done and can clarify which branch is the current truth and a small guide on how to do a proper build (ideally with how to install to a local maven repo; or in our case nexus). I'm willing to try to contribute to get this feature ready, if possible including the validation part.
We want to upgrade to Spring Boot 3.x and beside the migration process itself at the moment it seems that GWT not supporting Jakarta is the only (major) thing holding us back. So as mentioned before I want to try to push this as much as possible. |
I'm happy to report back that I am now able to build my GWT-using project with Jakarta EE 10 (using Payara 6.2023.6) and that some simple sanity testing works as desired. For anybody who ends up here trying to do the same, I'll note that in my previous attempt I had removed some JAR files from the way it had been built before — I had thought that they were cruft and/or that the change obviated their need and that was an error on my part. Thank you, @niloc132, for making that build available to me. |
@dodgex changing over the validation APIs to a new jar will need to be a separate project - GWT as a whole stalled on a fairly old version of the original validation API, but there are a few external projects that are much more up to date. If you aren't directly using the validation components of RequestFactory, you can just add the old validation API jars and leave the implementation alone. If however you need the new implementation as well, I'm not sure what the scope of that will be. Producing your own build is just a matter of following the readme - As far as this branch in general, I appreciate that you have a need to "push" this work forward, but I ask that you understand that we need support more than pushing ;). If you're able to help by picking up one or more of the remaining tasks to get this merged (or reviewing other pending PRs, or helping with the release testing process when the time comes for that), that will facilitate getting this release out the door more quickly. I work on GWT as time permits out of interest in the technology and community, but my company (Vertispan) is also available for hire to support GWT or teams that use it. @dbbernstein could you share the jars that you had understood to be cruft? A "stable" project like GWT certainly accumulates extra junk over time, but if you can point to some things that we should look to remove as we update (or things we should be more explicit about requiring), I'd appreciate it. As part of the next release cycle, we're going to have quite a few dependencies to update, so as to add Java 17 language support, and it seems like a good time to think about cleaning house. |
@niloc132 In addition to
|
I am pretty sure we are NOT using the regular client side(?) validation stuff, but we are using the validation part in requestfactory. We have a custom implementation of
After writing i realized, that I already knew how to run the build, but I'll check out the maven related parts. Thanks! |
I think i totally managed to mis-communicate my point. Of course I want to "push" the changes. But for sure I am willing to contribute as much as possible. This includes allocated company time, as well as some of my free time.
This is what I want to do. Due to personal and company interest I honestly am currently more focus on the jakarta part. Sadly I can't test the Jakarta Servlet Stuff in our Projects, as without Jakarta validation our suite of util projects does not build without removing the validation related part that is needed for the final projects. |
I will try to setup an environment to properly work on gwt and possibly more actively contribute to this. also I will look if i can estimate the impact of jakarta validation for requestfactory. What branch is the build you uploaded on 06-Jul-2023 based on? As far as I can tell there are 3 branches, but none of them seems to be "up-to-date".
|
I got a state where the gwt-servlet-jakarta is building with Jakarta Validation (3.0.2) running Done so far:
NOT done so far:
*) Unlike Jakarta Servlet, where replacing some packages was sufficient, Jakarta Validation has some new methods in its interfaces and classes. As of now most of the new methods are implemented as noop. My first goal was to have something that compiles. |
I created a branch based on the branch of sparsick where I commited current state of jakarta-validation https://github.com/dodgex/gwt/tree/9727-jakarta-servlet-support As of now, it only has one extra commit with the above mentioned changes |
@dodgex misunderstanding takes two of us - I might have gone overboard with trying to be clear about how we all contribute time/effort to get stuff done. Glad to have your input and also any work you can add. Would you consider creating a new issue at the issue tracker specifically for discussing jakarta.validation? I fear it will be more complex than servlets, especially for requestfactory, due to the need of supporting the same API on the client and server (whereas changing the servlet API requires only changing the server) - rather than 2 jars that are "used the same way", we end up with potentially 4 sets (and not just gwt-servlet and rf-server, but gwt-user, rf-client, rf-shared too), and have to deal with any API changes along the way... Note that client-side validation within GWT itself is presently deprecated, and there are external projects that have done some work to update and modernize this. At this time though, https://gitlab.com/ManfredTremmel/gwt-bean-validators still appears to use the javax packages (and off the top of my head I'm not seeing the other one I was thinking of...) -- I've had a busy summer for personal reasons, and will try to get a fully up to date branch soon. The current bleeding edge is (not including known-missing work):
If all goes well, I'll push a commit, snapshot build, and javadoc update this weekend to reflect those things. |
done gwtproject#9844
From raw code point of view it should be no more than those 3 changes, the rest is for client side validation. https://github.com/dodgex/gwt/blob/fa50f5bd14aa67691a18263367dba131b55022ba/user/build.xml#L159
when you are talking about "potentially 4 sets" compared to "2" you don't just mean the client side impact and related jars, but mixing e.g. In either case, maintaining two APIs is painfull and can be error-prone, especially when its done by search&replace in the build. Not to mention the fact that it is not only package names but also "injecting" code in 1 (rf only) to 25+ (gwt validation) locations.
I already saw that in the documentation. But I assume, that this does not mean, that we can get rid of it :D
The 1.x releases are still on javax but there is a 2.0.x with jakarta.
👍 |
If we can then I think what you propose is probably doable. I think that this scope and "use cases that matter to us" discussion should be taken up at the new issue. -- GwtServletBase is indeed missing from my javadoc, but was put in the new jar... because I flubbed the package name. Say what you will about gradle (trust me, I've said it myself), it would have done a better job here. -- I've merged this branch along with my changes on top, my branch 9727-jakarta-servlet-support is now the "source of truth" for this feature. I'll make an upstream draft PR, listing the known-incomplete aspects to land this, and push out some new build artifacts/docs. |
Hey @niloc132 , had two rough weeks... I just managed to dedicate some time last weekend to rebase my changes to your current state of the 9727 branch and did some testing of requestfactory related validation stuff in a spring boot 3 context. as mentioned in gwtproject#9844 it requires some java API emulations (I used Also I triggered a full build using the github actions and it looks like everything was OK (ignoring the fact that it shows error annotations in the Today I want to try to get a sample project with client side gwt validation to run just to have some extra confirmation that it works. Beside the planned tests, I am honestly not sure how to continue this effort. In my current branch state I have NOT moved the files updated into a jakarta package as done before (to first get it to work at all...). If my tests come to a "it seems to work" result, i can do the moving. but then there is still the question, how to handle the diffrent jars? also, where is the best place to continue the communicatins? here, or in the issues/PR in gwtproject/gwt? |
Shortly after my last comment I realized that my test project used javax.validation on the client side for the requestfactory |
I'm happy to have conversation continue here, but it probably makes sense to post a summary to the issue at least, for somehting more official/concise, less conversational/experimental. Also consider joining gitter/matrix chat, we can even make a specific room for the topic if we get too noisy in the main channels (e.g. https://matrix.to/#/#gwtproject_gwt:gitter.im) Build failure: According to your link, the "failure" was in reporting the (successful) test results, somehow it can't get a github actions token to be able to publish. Specific info in that build log: https://github.com/dodgex/gwt/actions/runs/5629630470/job/15254805815#step:6:29 I don't have any solid answers/advice at this time about how/what to move, I've barely had time to work on the jakarta.servlet stuff. If nothing else, perhaps if you make enough progress you could release on your own groupId and get a little more testing in production by other projects, then we fold the changes back in to upstream gwt? |
Ofc I could do that. At least for internal work I wanted to do it anyway, to test with our company projects. but as I mentioned I had trouble to get builds for anything that is not gwt-servlet-jakarta with the jakarta code. If by any chance you could help me out with that it would be awesome. |
At least gwt-user and the client requestfactory jars assume the 1.0.0.GA version of validation-api, and so must be using javax.validation - we can't change GWT itself over to use the de.knightsoft-net packages as far as I know (as I think those depend on GWT, so we'd have a circular dependency) - plus, doing so would require removing gwt's own validation generation (and tests, etc). I don't think you're going to find that gwt-dev or codeserver themselves use validation code (but gwt-dev facilitates its use, when developers run a server inside of the dev mode server, rather than developing an external server). By the same token, gwt-dev definitely has javax code, but just for servlets (and we presently do not plan to remove that as part of gwtproject#9845 as this would break the "use dev mode as a server" use case). |
Well, isn't this point of the change to use Jakarta Validation?
I never intended that; at the moment I use it, as it provides the supersource/emulation classes for some
While it could make things easier, it is not intended or required.
Again I feel like I managed to not accurately communicate my idea/intention. I do not intend to remove or break any current features (other than replacing javax with jakarta). We could try to upgrade gwt-dev/codeserver to jakarta too, but I am kinda sure that this would be a lot more effort, especially if it has to be done while keeping javax support like we do at the moment with gwt-servlet. although it could prevent unwanted All I asked for, is support to build a version of |
After realizing there is even more stuff in
"a seperate project" does not necessarily mean it must be an official project. :D For now I'll focus my work on getting the RequestFactory stuff to work (with both jakarta servlet and validation) and test it in our company projects. When it reaches a state where I think it could be stable enough to provide builds for others to test, I see if i can somehow setup a public repo containing the files for others to test as you suggested. I'd like to hear your opinion regarding my gwt validation comment above. but at this moment I can't justify using company time to work on a feature that is unmaintained in upstream and announced to be moved out of the core sdk. especially as it is not used by us. Depending on your comment, I would like to suggest promoting gwt-bean-validators as a replacement for the current implementation in my Jakarta Validation Issue. If for any reason it becomes a requirement to keep full validation updated even in this state, we will find a way to deal with it. But without a clear decision I see no value in working on this feature right now. |
Today I managed to build a dedicated jakarta variant of gwt-user and the requestfactory jars. The content feels not yet 100% as intended, but I think it is a good step. Are the top level Also I realized, that even when running the tests with |
Another idea regarding validation, maybe it could be an Option to drop it only for jakarta jars. This way projects staying on the javax variant will not loose it. |
Partial #gwtproject#9727