search

NOAA-EMC / NCEPLIBS-bufr

The NCEPLIBS-bufr library contains routines and utilites for working with the WMO BUFR format.
Other
44 stars 19 forks source link

Only download test files if make test is used #517

Closed edwardhartnett closed 1 year ago

edwardhartnett commented 1 year ago

Currently the test data are downloaded for make.

They should only be downloaded when make test is used. In many cases make test is not run and therefore no need to download the tarball. Also doesn't this mean that without FTP, the release cannot be built? That's not going to work.

@AlexanderRichert-NOAA and @Hang-Lei-NOAA is building NCEPLIBS-bufr-12.0.0 possible on WCOSS? I don't think it has FTP.

Hang-Lei-NOAA commented 1 year ago

My build of bufr/12.0.0 on wcoss2 seems fine now. WCOSS2 is fine with some ftp. NCO will check the ftp in the code review.

On Sun, Jul 30, 2023 at 10:16 AM Edward Hartnett @.***> wrote:

Currently the test data are downloaded for make.

They should only be downloaded when make test is used. In many cases make test is not run and therefore no need to download the tarball. Also doesn't this mean that without FTP, the release cannot be built? That's not going to work.

@AlexanderRichert-NOAA https://github.com/AlexanderRichert-NOAA and @Hang-Lei-NOAA https://github.com/Hang-Lei-NOAA is building NCEPLIBS-bufr-12.0.0 possible on WCOSS? I don't think it has FTP.

— Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/NCEPLIBS-bufr/issues/517, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKWSMFA5ZACY3RJAEXNSBKDXSZUCPANCNFSM6AAAAAA25HEGGE . You are receiving this because you were mentioned.Message ID: @.***>

AlexanderRichert-NOAA commented 1 year ago

Would it be a sufficient alternative for the builder to include -DBUILD_TESTS=OFF in the cmake command?

edwardhartnett commented 1 year ago

Not really. We have this FTP test file download in a bunch of the NCEPLIBS. They should all work the same and use the exact same cmake options. NCEPLIBS-g2 should be used as the pattern.

jbathegit commented 1 year ago

They should only be downloaded when make test is used. In many cases make test is not run and therefore no need to download the tarball.

By make test do you mean ctest? Or are you talking about something different?

edwardhartnett commented 1 year ago

If you download the files from the tests/CMakeLists.txt file, then they will only be downloaded when make test, or ctest, is run.

Check out the NCEPLIBS-g2 for an example.

jbathegit commented 1 year ago

I will, b/c I remember you pointed that out before as an example to follow. I'm just not sure how quickly I can get to it.

FWIW, right now the tar file is already defined for downloading within tests/CMakeLists.txt. However, for whatever reason it actually gets downloaded during the cmake step, and not during the ctest step. So I'll have to take a lot closer look to see what the differences really are in tests/CMakeLists.txt between NCEPLIBS-bufr and NCEPLIBS-g2.

jbathegit commented 1 year ago

I just took a quick peek, and it looks like there's already an option BUILD_TESTS in the main directory CMakeLists.txt file which is set to a default value of ON. So wouldn't an easier solution to this issue be to just inform users that they can include the option -DBUILD_TESTS=OFF when they run the cmake step?

Alternately, we could instead change the default value to OFF so that users don't have to specify anything if they don't want to run ctest, but then we'd need to inform them to set -DBUILD_TESTS=ON if they want to run the tests. In this case we'd also need to add that same define flag to the cmake step of each of the .github/workflows/*yml files so that the automated CI testing still runs for each future PR.

Or am I missing something here?

edwardhartnett commented 1 year ago

BUILD_TESTS=OFF still causes the files to be downloaded. This is happening for example in CI builds that need bufr, and that's wasting time and electricity.

So like g2 we need to add an option:

option(FTP_TEST_FILES "Fetch and test with files on FTP site." OFF)

Then only download the test files if this option is set to on. That means many of the tests will not be able to run without this option set to on, but that's OK.

We don't want to turn off tests by default, even if that would solve the download problem, which it would not.

Make sure you use the exact same option as other NCEPLIBS to reduce confusion. We want to do everything in a standard way, so if you think you want to do something different from the other NCEPLIBS in this regard, we need to evaluate, and, if you do come up with a better way to do things, we need to apply it everywhere, not just in NCEPLIBS-bufr.

jbathegit commented 1 year ago

BUILD_TESTS=OFF still causes the files to be downloaded. This is happening for example in CI builds that need bufr, and that's wasting time and electricity.

Make sure you use the exact same option as other NCEPLIBS to reduce confusion. We want to do everything in a standard way, so if you think you want to do something different from the other NCEPLIBS in this regard, we need to evaluate, and, if you do come up with a better way to do things, we need to apply it everywhere, not just in NCEPLIBS-bufr.

@edwardhartnett as a baseline check I just did an NCEPLIBS-bufr build on WCOSS2 with BUILD_TESTS=OFF, and it did exactly what I expected it to do, which is to say it did not download the tarfile from the EMC ftp server nor try to build any tests.

So I still don't understand why we need to add a new FTP_TEST_FILES option in the first place, or why you believe that BUILD_TESTS=OFF would still cause the tarfile to be downloaded. I get your point about having consistency across all of the NCEPLIBS, so I can go ahead and add the FTP_TEST_FILES option if you really feel strongly about this, but it seems to me it's just adding more unnecessary complexity and potential for confusion, at least as far as NCEPLIBS-bufr is concerned. We already have the BUILD_TESTS and TEST_FILE_DIR options which seem like they should be more than enough to cover all possible scenarios, including copying the tarfile from a local directory rather than downloading it from a remote server if a user wants to do that before running any tests. So what added value do we really get by adding yet another option FTP_TEST_FILES on top of those?

Bottom line - if we want a consistent approach across all libraries, then maybe a simpler (and therefore better? ;-) approach is to just have the two options BUILD_TESTS and TEST_FILE_DIR in all of them, unless you can think of an example where having a third option FTP_TEST_FILES on top of those first two is really adding anything helpful to the mix. If you can think of such a case, then by all means please correct me.

AlexanderRichert-NOAA commented 1 year ago

I just noticed that in the w3emc CI, the variable being set is -DBUILD_TESTING, not -DBUILD_TESTS, so I think that may be the issue as far as why it's still downloading the test data. That said, I would be in favor of changing BUILD_TESTS to BUILD_TESTING for consistency with other NCEPLIBS. Unless there's some reason to do otherwise, I've been moving various libraries to just use include(CTest) in the root CMakeLists.txt without separately defining BUILD_TESTING, because that variable gets automatically created and enabled with include(CTest). See https://github.com/NOAA-EMC/NCEPLIBS-sp/blob/develop/CMakeLists.txt as an example (no BUILD_TESTING option up top, just the test block down below).

edwardhartnett commented 1 year ago

OK, I agree about the BUILD_TESTING and the use of this idiom:

# Add tests.
include(CTest)
if(BUILD_TESTING)
  add_subdirectory(tests)
endif()

This seems to be what we use in most or all of the other NCEPLIBS, but not NCEPLIBS-bufr. So how about I submit a PR to change that right now...

edwardhartnett commented 1 year ago

OK, once that change is made, I can do a build of bufr with BUILD_TESTING=OFF and now the files are not downloaded. Excellent.

jbathegit commented 1 year ago

Works for me. Once the developer CI finishes I'll go ahead and merge it. Thanks to both of you!

jbathegit commented 1 year ago

OK it's done. Let me know if you still see any issues.

Hang-Lei-NOAA commented 1 year ago

The buf2/12.0.0 has been on wcoss2 both dogwoods and cactus /apps/ops/para/libs/modulefiles/compiler/intel/19.1.3.304/bufr/12.0.0.lua Please load as hpc-stack way to check. Then they will install as NCO way.

On Wed, Oct 4, 2023 at 9:12 AM Jeff Ator @.***> wrote:

OK it's done. Let me know if you still see any issues.

— Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/NCEPLIBS-bufr/issues/517#issuecomment-1746851735, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKWSMFBCIB3FOFQZLBR2E33X5VOCRAVCNFSM6AAAAAA25HEGGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBWHA2TCNZTGU . You are receiving this because you were mentioned.Message ID: @.***>

jbathegit commented 1 year ago

The buf2/12.0.0 has been on wcoss2 both dogwoods and cactus /apps/ops/para/libs/modulefiles/compiler/intel/19.1.3.304/bufr/12.0.0.lua Please load as hpc-stack way to check. Then they will install as NCO way.

Thanks @Hang-Lei-NOAA and I'll take a look, but I'm also going to move this discussion over to a more appropriate message thread.