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

DS-2437: Ensure all webapps unpack "additions" into their WEB-INF/classes #1331

Merged
merged 1 commit into from Mar 23, 2016

Conversation

tdonohue
Copy link
Member

@tdonohue tdonohue commented Mar 4, 2016

https://jira.duraspace.org/browse/DS-2437

This PR fixes our issues with loading the "additions.jar" in WebApps in Tomcat 8 or Glassfish. It does the following:

Ensure all webapps unpack org.dspace.modules.additions (and any transitive dependencies) into their WEB-INF/classes/. This ensures classes in "additions" are loaded prior to any other JARs, per Servlet 3.0 spec, section 10.5:

"The Web application class loader must load classes from the WEB-INF/ classes directory first, and then from library JARs in the WEB-INF/lib directory."

This work was a collaboration between myself and @rradillen (see notes below)

@tdonohue tdonohue added the bug label Mar 4, 2016
@tdonohue tdonohue added this to the 6.0 milestone Mar 4, 2016
@mwoodiupui
Copy link
Member

I've read it but not yet tried it. This seems quite straightforward.

@mwoodiupui
Copy link
Member

It builds and installs cleanly. Only one copy of additions-VERSION.jar remains, in [DSpace]/lib, where presumably it's not a problem since that won't be on any webapp.'s class path. That overlay still works: I hacked the script launcher to add "This launcher is hacked" to the help output and the message appeared as expected.

I hacked a copy of the DSpaceConfigurationManager in dspace/modules/additions to append "(hacked)" to the value of dspace.name. I can see the class file in webapps/xmlui/WEB-INF/classes, and in the XMLUI Control Panel Configuration tab I can see the expected alteration of the site name. I tried this in both Tomcat 7 and Tomcat 8.

However, something strange is going on with bin/dspace when DSpaceConfigurationManager is overlaid: it cannot find the configuration. I put some debugging writes into DSpaceConfigurationManager.getDSpaceHome() and this is what I got:

mhwood@toolshed ~/dspaces/DS-2437 $ bin/dspace
Trying providedHome: null
Trying relative to JAR: /home/mhwood/dspaces/DS-2437/webapps/oai
Trying catalina: null
Trying user.home: /home/mhwood
Trying /
Failure during kernel init: DSpace home directory could not be determined. It MUST include a subpath of '/config/config-definition.xml'. Please consider setting the 'dspace.dir' system property or ensure the dspace-api.jar is being run from [dspace]/lib/.

There is some magic in bin/dspace to include dspace-oai in the classpath for some reason. We may need to do that another way. And this may have no connection with the present PR. It looks like this may be related instead to DS-2423. It could also be that copying classes from dspace-services into "additions" causes weirdness.

Supposing that the bin/dspace breakage is unrelated, I'm +1 for this change.

@mwoodiupui
Copy link
Member

So far I'm unable to test on Glassfish, probably because we depend on several org.glassfish.jersey** artifacts which are, unsurprisingly, provided by Glassfish and thus tangled in classloader complexities.

@mwoodiupui
Copy link
Member

I tried rearranging the classpath in bin/dspace so that lib/ came before webapps/oai/WEB-INF/classes and that resolved the problem. I am more confident now that the bin/dspace issue is not relevant to this PR.

@KevinVdV
Copy link
Member

Alternative PR: #1333

@tdonohue
Copy link
Member Author

I've reworked this PR based on @rradillen's work #1333. I prefer that approach, but there was some necessary minor cleanup to that PR (which is why I didn't merge it immediately). I limited unpack-dependencies to matching specifically org.dspace.modules.additions, and added a commented-out option to exclude transitive dependencies, in case that becomes necessary for any users.

I've closed #1333 as those fixes have now been applied into this PR. Thanks to @rradillen!

@tdonohue
Copy link
Member Author

This seems to have general approval. I've tested @rradillen's improvements (in #1333) and merged them into this PR.

Merging this into master.

@tdonohue tdonohue merged commit e066cd5 into DSpace:master Mar 23, 2016
@tdonohue tdonohue deleted the DS-2437 branch March 23, 2016 19:57
pnbecker added a commit to subugoe/dspace-development that referenced this pull request Sep 22, 2021
…sses

We backported DSpace/DSpace#1331 from DSpace 6
to DSpace 5. It enables us to override classes from dspace-api using
dspace/modules/additions even if Tomcat >= 8 is used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants