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

Load Resources with Guava API #200

Merged
merged 21 commits into from
Sep 18, 2024

Conversation

SethChampagneNRL
Copy link
Contributor

Description:

Compiling ERDDAP source into a jar file does not work due to the way ERDDAP accesses several static resource files. This change is to move the classpath resources into the main/resources source set, and patch the code to load classpath resources using the Guava Resources API. This fixes some of the assumptions that ERDDAP makes about the WEB-INF/classes directory, and allows the ERDDAP source to be bundled into builds as a jar file dependency.

Resource files outside of the classpath remain in place. The code will still use filesystem paths for other directories within WEB-INF (e.g. cptfiles, ref, etc). This change should not impact any existing ERDDAP deployments since all files can remain where they are in the war artifacts.

Changes:

  • Remove classPath and webInfParentDirectory static variables from File2
  • References to webInfParentDirectory will all use the variable from EDStatic
  • Change ERDDAP to load classpath resources using Guava Resources utility
  • Continue loading non-classpath resources via file paths relative to WEB-INF directory
  • Move resources to main source set following Java standard practice
  • Allows access to resources within jar files, adding support for alternative build DevOps

I wanted to additionally change the File2 WEB-INF directory lookup to use the ServletContext, but this would require changing the way EDStatic is initialized and make it dependent on the Erddap.init() method instead of the Erddap constructor. So WEB-INF parent directory is still located using the classpath of String2.java.

Copy link
Contributor

@ChrisJohnNOAA ChrisJohnNOAA left a comment

Choose a reason for hiding this comment

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

Many tests are failing when run through maven mvn test for me with these changes.

Also, a few questions.

@SethChampagneNRL
Copy link
Contributor Author

I am working on an update to address these issues. The unused imports were from refactoring and I forgot to remove them. I will admit that I am not familiar with Maven so I didn't know how to run the tests, but I have those running now and I will fix the initialization problems. For the SSR, I didn't realize it is also decompressing the stream, but I can update the code to handle that as well. I hope to have all these wrapped up soon. Thanks Chris!

@srstsavage
Copy link
Contributor

srstsavage commented Sep 17, 2024

Haven't looked closely but I'm getting a single compilation error when running tests after the latest commits:.

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.13.0:testCompile (default-testCompile) on project ERDDAP: Compilation failure
[ERROR] /the/path/erddap/src/test/java/gov/noaa/pfel/coastwatch/util/HtmlWidgetsTests.java:[24,28] cannot find symbol
[ERROR]   symbol:   method webInfParentDirectory()
[ERROR]   location: class com.cohort.util.File2

Excited for these changes by the way. I'd been thinking about ways to get additional custom jars into ERDDAP, but the ability to compile ERDDAP as a jar usable in other projects is a great solution to customization!

Copy link
Contributor

@ChrisJohnNOAA ChrisJohnNOAA left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes.

WEB-INF/classes/com/cohort/util/XML.java Outdated Show resolved Hide resolved
@SethChampagneNRL
Copy link
Contributor Author

I am sorry that my initial pull requests had all of the tests failing but they pass now when I run locally. I brought back the File2 static initialization for tests and File2.getClassPath() for some particular parts of the code that were difficult to workaround.

Two notes:

  • The git-code-format-maven-plugin is not working for me. I get a "No valid maven executable found !" message, but this is probably user error on my side. I worked around this by commenting out the plugin in pom.xml when I run maven.
  • I also had to switch EDStatic.useSaxParser = false; in the test Initialization. The new SAX parser seems to cause three of the EDD tests to error. With SAX turned off, I still had only one test error, the EDDGridTests.testGeotif2(), and this was due to a 503 error from https://oceanwatch.pfeg.noaa.gov/thredds/dodsC/satellite/MH/chla/8day.das

I built the erddap.war using mvn package and I verified it working in tomcat 10. Are there any other changes or documentation updates I should make for this pull request?

@srstsavage
Copy link
Contributor

srstsavage commented Sep 18, 2024

Still getting one more error when running test, double checking now...

[ERROR] Errors:                                                                                          
[ERROR]   EDDGridTests.testGeotif2:266 NullPointer Cannot invoke "gov.noaa.pfel.erddap.dataset.EDDGrid.className()" because "gridDataset" is null                                                                  
[INFO]                                                                                                                                                                                                             
[ERROR] Tests run: 347, Failures: 0, Errors: 1, Skipped: 1 

Ah, looks like we posted at the same time and this is a known issue!

@ChrisJohnNOAA
Copy link
Contributor

I am sorry that my initial pull requests had all of the tests failing but they pass now when I run locally. I brought back the File2 static initialization for tests and File2.getClassPath() for some particular parts of the code that were difficult to workaround.

Two notes:

  • The git-code-format-maven-plugin is not working for me. I get a "No valid maven executable found !" message, but this is probably user error on my side. I worked around this by commenting out the plugin in pom.xml when I run maven.
  • I also had to switch EDStatic.useSaxParser = false; in the test Initialization. The new SAX parser seems to cause three of the EDD tests to error. With SAX turned off, I still had only one test error, the EDDGridTests.testGeotif2(), and this was due to a 503 error from https://oceanwatch.pfeg.noaa.gov/thredds/dodsC/satellite/MH/chla/8day.das

I built the erddap.war using mvn package and I verified it working in tomcat 10. Are there any other changes or documentation updates I should make for this pull request?

I'm working on deflaking tests in a separate branch: https://github.com/ChrisJohnNOAA/erddap/tree/ReduceTestFlakiness
I disable the geotif2 test there due to the unreliability of that external dependency. I think the other tests you mention are fixed in that branch.

Given that, I think this is good to go.

@ChrisJohnNOAA ChrisJohnNOAA merged commit ea5c5c5 into ERDDAP:main Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants