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
Using an embedded tomcat to start the application #8797
Conversation
…T integration tests
…tyExpressionHandler
@LucaGiamminonni : One immediate question. How does this impact the old (v6) REST API (in To be clear, I am a big fan of this PR & the movement towards an embedded Tomcat. However, it's unclear to me if we can make this embedded Tomcat change in DSpace 7, or if we'd need to wait for DSpace 8 when the old REST API is removed. (Rereading your description... I realized that it sounds like we can still deploy the |
I'm going to test this PR later today, this is exciting, thanks, @vins01-4science ! |
I can confirm that this PR now builds correctly in Docker-Compose, and results in a running DSpace backend instance. The build works flawlessly:
and then it starts up without any errors:
Now I need to reconfirm that the embedded Tomcat version still works. I'll check that and report back, probably tomorrow. But this is looking very promising indeed. @tdonohue will be pleased, I'm sure. |
I've made an attempt at testing the embedded Tomcat, and am stuck. I'm trying to do this from the CLI, working from the comments above, but I can't seem to get past this error https://gist.github.com/hardyoyo/ba6085196aab0cf9907549ebfb4111bc when running these commands:
There may be another way to override the dspace.dir property, I'll see if I can figure it out. |
I've tried a few options, can cannot see a way to provide an override for the dspace.dir property for the server-boot jar. The inheritance from the dspace parent pom causes dspace.dir to be hardcoded in the server-boot JAR at build time, preventing runtime overrides. If I attempt to globally override the dspace.dir property at build time, the additions module fails to build. OH, after typing all that, I realize I can just skip the additions module. This seems to work:
well... kinda... it still doesn't build successfully. This is a bit of a tangle, isn't it? |
I tried again and if you provide both
Could you try with this one? Moreover, if we want to just run it without any parameter we need to change the building phase of the jar to just include it, take in count that with that configuration the |
I've tried this: java -jar ./server-boot-8.0-SNAPSHOT.jar --spring.config.location=file:///Users/hpotting/d/install/dspace7/application.properties with dspace.dir set correctly in that application.properties file (copied out of the additions module's target) I get the same error, the embedded tomcat can't find the log config. Can you provide an example of what you found that works? You mentioned:
Can you show me what you mean by that? Thanks! |
Sure, here you got the command:
In this way you are overriding the default configuration of the standard
|
I think what we really need are instructions for testers of this PR. I've tried to get it running, and have gotten close, but still have not completely spun up the server-boot jar. I can't spend any more time on this today, I hope to be able to return to it later in the week. I really want this PR to be merged, because I feel very strongly that this is important for the project. Instructions would help not only me, but any other interested committers, or other community members. |
OK, I managed to spin up the embedded tomcat. Here's what I needed to do:
step 3 I'm not entirely sure how important it is... but it's what I did. Alright, I'm confident this all works. We need good docs. I'm going to give this PR a +1 |
* @author Luca Giamminonni (luca.giamminonni at 4science.it) | ||
* | ||
*/ | ||
@SuppressWarnings({ "checkstyle:hideutilityclassconstructor" }) |
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.
While I generally am leery of suppressing checkstyle warnings, in this instance I think it's fine. Calling it out because I noticed it, but I think it's fine to leave this as-is. (A quick search of the entire codebase finds a number of other checkstyle SuppressWarnings invocations, none for this particular warning).
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.
This all looks fine, the Docker build/testing instructions still work for the war build, and the embedded tomcat works as well. I think we'll need lots of docs on how to best make use of the server-boot/embedded tomcat (with instructions for various IDEs). This is a nice improvement for DSpace 8, and will surely lead to improved deployment methods in the future. 👍
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.
@vins01-4science and @LucaGiamminonni : Thank you for this! I finally got back to reviewing/testing this today. Overall, it looks great, but there are some suggested updates.
The largest is requesting the removal of the changes to GeneralAuthorizationFeatureIT
as they seem unrelated to embedded Tomcat and make up the majority of this PR (over 300 lines of changes). If they are related, then they need to be described. If they are not related, then I'd recommend submitting them as a different PR and explaining the purpose better....a 300 line change shouldn't be embedded within an unrelated PR. It needs separate attention.
Beyond that, this PR works very well. I've tested that the basic install process is unchanged (mvn clean package; ant fresh_install; deploy via Tomcat
). I've also tested that you can alternatively run java -jar .\server-boot-8.0-SNAPSHOT.jar
instead of the deploy via Tomcat (but obviously, first you still must install everything via Ant...but that's expected).
In both scenarios, I've verified that the User Interface and Hal Browser appear to still work with no issues. I've also verified I can use java -jar .\server-boot-8.0-SNAPSHOT.jar --dspace.dir=[directory-path]
to run the JAR against a DIFFERENT dspace installation.
A few sidenotes for the future (not required in this PR):
- After this PR is merged, I think we would need to make some additional updates to the following: Ant scripts (to remove filtering of application.properties from Ant) and Install Documentation (to add this new option).
- As a future task (likely post-8.0), it'd be wonderful to find a way to remove Ant altogether, so that the runnable JAR can create the
dspace.dir
directory structure (i.e. run it's own "fresh_install"). That'd allow us to potentially distribute the entire backend as a JAR (no need to build it unless you need to customize it).
...er-webapp/src/test/java/org/dspace/app/rest/authorization/GenericAuthorizationFeatureIT.java
Outdated
Show resolved
Hide resolved
It just came to my mind the Handle Server... would this PR has any affect on how the Handle server integration is currently launched? In the same way, what about the CLI? |
@paulo-graca : No, this PR has no impact on anything other than Tomcat. With this PR in place you have two webapp install options:
However, this PR has no impact on the DSpace installation directory ( |
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.
👍 Thank you @vins01-4science and @LucaGiamminonni ! This all looks good to me now. I cannot find any bugs with the approach taken when testing using the UI.
Obviously, this will require updates to our Installation Documentation: https://wiki.lyrasis.org/display/DSDOC8x/Installing+DSpace
If you have time, I'd appreciate help there. In the meantime, I'll flag with with a needs documentation
label to remind us it requires docs.
Great work! |
Hello @tdonohue, the installation procedure has been updated with a new deploy step involving the executable jar configuration. However, the other changes made with this PR are involving mainly the development procedure (Development Experience), but I cannot find any page that contains this kind of information. Thank you. |
@vins01-4science : Thanks for the updated installation docs! Regarding developer documentation, you are correct that we don't tend to add that into the official documentation space (DSDOC8x). However, we do have some general IDE docs like these:
If you could find time to document just one IDE (whichever one you tend to use), that might be helpful to others in how to use this feature for development. So, if you tend to use IntelliJ, maybe just update that first page? Or, honestly, if you feel those pages are way to outdated, just create a brand new page under this one https://wiki.lyrasis.org/display/DSPACE/Developer+Guidelines+and+Tools My main goal is to just find a way to better document this for feature for development. So, where you create the page doesn't matter much... I can always move it later. |
Documentation created here: https://wiki.lyrasis.org/display/DSPACE/DSpace+8+development+with+IntelliJ+IDE Note that the configuration / steps can be reused for pretty much every IDE, since basically we need just to start a standard Java Application and its main method 👍 |
The purpose of this PR is to allow dspace backend to be started using an embedded tomcat or via an executable jar. It still allows DSpace backend to also run in an external Tomcat (as currently).
Major benefits to this PR
To allow that I made the following changes:
@SpringBootApplication
annotation since the spring boot modules will be others (server and the new server-boot)With these changes you can start the DSpace backend in 3 ways
Prior to running any of these, you FIRST must install the DSpace backend as normal using
mvn clean package; ant fresh_install
. Those steps are still required.server
project in a Tomcat installationserver-boot
project (placed under the target folder). For examplejava -jar .\server-boot-8.0-SNAPSHOT.jar
(this runs the backend using an embedded Tomcat)These modifications had the side effect of modifying the order in which the beans are discovered and injected into lists annotated with autowired: for example, in DSpacePermissionEvaluator plugins can be inserted in an unspecified order. This highlighted a few issues which I fixed.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
pom.xml
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.