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

Change to using netcdf modules instead of netcdf-all #144

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

ChrisJohnNOAA
Copy link
Contributor

@ChrisJohnNOAA ChrisJohnNOAA commented Apr 11, 2024

Description

This loads netcdf dependencies as individual modules instead of the old way that needed to download netcdf-all outside of the normal dependency management.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • [ X] Bug fix (non-breaking change which fixes an issue)

Checklist before requesting a review

  • [ X] I have performed a self-review of my code
  • [ X] My code follows the style guidelines of this project
  • [X ] I have commented my code, particularly in hard-to-understand areas
  • [ X] I have made corresponding changes to the documentation
  • [ X] My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • [X ] New and existing unit tests pass locally with my changes

@srstsavage
Copy link
Contributor

Very happy to see this as the current wget approach in the pom wasn't working for me and I was just starting to put together some other possible approaches/band-aids. Obviously moving to the official set of dependency offerings from Unidata is the best path forward.

Tests pass for me locally (Linux, Maven 3.6.3, Java 17.0.3), with the exception of these which have already been identified in #140.

[ERROR] Errors:                                                                                          
[ERROR]   HashDigestTests.basicTest:21 » Runtime 
ERROR in Test.ensureEqual(Strings) line #1, col #1 '3[end]'!='9[end]':
Specifically, at line #1, col #1:
s1: 327fbb2aa6c6297d4fdd5fdf4b14e289[end]
s2: 96e9b4d974b734874a66f433b276a41a[end]
    ^

[ERROR]   NcHelperTests.testUnlimited:578 » FileNotFound /temp/unlimited.nc (No such file or directory)
[ERROR]   TableTests.testReadASCIISpeed:2558 Simple readASCII took too long (time=135ms > 130ms) (but often does when computer is busy).
[INFO] 
[ERROR] Tests run: 57, Failures: 0, Errors: 3, Skipped: 0

That's good enough for a 👍 from me!

Copy link
Contributor

@srstsavage srstsavage left a comment

Choose a reason for hiding this comment

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

Looks great!

@ChrisJohnNOAA ChrisJohnNOAA merged commit 5f232b9 into ERDDAP:main Apr 11, 2024
@BobSimons
Copy link
Collaborator

BobSimons commented Apr 11, 2024 via email

@mwengren
Copy link
Contributor

@ChrisJohnNOAA One dependency that may be missing is an SLF4J implmentation.

When I run ERDDAP locally, I'm getting the following SLF4J warning messages now:

////**** EDStatic Low Level Startup
localTime=2024-04-12T15:35:41-04:00
erddapVersion=2.23
Java 18.0.2.1 (64 bit, Oracle Corporation) on Linux (4.15.0-221-generic).
MemoryInUse=   133 MB (highWaterMark=   133 MB) (Xmx ~= 2048 MB)
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
logLevel=info: verbose=true reallyVerbose=false
%%% TouchThread: new touchThread started at 2024-04-12T15:35:41-04:00
ERROR in File2.deleteIfOld: dir=data/dataset/_FileVisitor/ isn't a directory.
bigParentDirectory=data/
...

Not sure if this is a real issue or not (ERDDAP is running for me), but SLF4J is mentioned as a separate dependency that should be included in the Unidata docs for netcdf-Java.

I don't see an SLF4j dependency included anywhere in the current pom.xml.

@ChrisJohnNOAA
Copy link
Contributor Author

@mwengren I have a fix that here: #146

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.

4 participants