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

Jump to bottom

Running regression tests litters examples with empty .nmlcache directories #139

Closed
matthijskooijman opened this issue May 11, 2020 · 8 comments
Closed

Running regression tests litters examples with empty .nmlcache directories #139

matthijskooijman opened this issue May 11, 2020 · 8 comments

Comments

@matthijskooijman
Copy link
Contributor

I noticed this building the 0.5.1 Debian packages, which produced empty .nmlcache directories in the installed examples.

I think this is triggered by #125, which causes the examples to be built, changing the current directory to the examples directory. By default, nml seems to create .nmlcache in the current directory (but for these examples nothing is written to these directories).

I suspect that an .nmlcache directory was created for the regression test already prior to #125, but since the current directory was not changed, that was created in the repo root, which is not included on installs, so these did not show up in the Debian package.

To fix this, I would suggest:

  1. Using --cache-dir to use a single cache directory outside of the example dirs themselves,
  2. cleaning that directory and
  3. also pruning it from the install

I guess 1. could be omitted, but that makes 2. and 3. a bit more tricky.

This could maybe look like this (untested, though):

diff --git a/MANIFEST.in b/MANIFEST.in
index 5082182..a5bda1e 100644
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -14,6 +14,7 @@ include regression/beef.wav
 prune regression/output
 prune regression/output2
 prune regression/nml_output
+prune regression/.nmlcache
 
 # Include (some) examples
 recursive-include examples *.nml *.lng *.png
diff --git a/regression/Makefile b/regression/Makefile
index c1b335c..38ff734 100644
--- a/regression/Makefile
+++ b/regression/Makefile
@@ -8,8 +8,9 @@ _SE ?= echo
 TEST_FILES = $(basename $(shell ls *.nml))
 EXAMPLES = $(shell ls ../examples)
 NMLC ?= $(abspath ../nmlc)
+NML_CACHE_DIR = $(abspath .nmlcache)
 # Note: Manually overriding NML_FLAGS may break the regression test
-NML_FLAGS ?= -s -c --verbosity=1
+NML_FLAGS ?= -s -c --verbosity=1 --cache-dir=$(NML_CACHE_DIR)
 
 .PHONY: $(TEST_FILES) $(EXAMPLES) clean
 
@@ -50,4 +51,4 @@ $(EXAMPLES):
        $(_V) diff expected/example_$@.grf output2/example_$@.grf
 
 clean:
-       $(_V) rm -rf output nml_output output2
+       $(_V) rm -rf output nml_output output2 $(NML_CACHE_DIR)
@FLHerne
Copy link
Contributor

FLHerne commented May 11, 2020

I see this happens, and your explanation is correct.

(except that the existing regression tests created regression/.nmlcache, not in the root dir).

Your patch looks sane to me, but doesn't seem to work; the offending dirs are still created. I haven't figured out why yet. ;-)

@FLHerne
Copy link
Contributor

FLHerne commented May 11, 2020

I remember now that I used -n specifically to avoid this problem. Apparently it doesn't work.

-n, --no-cache Disable caching of sprites in .cache[index] files,
which may reduce compilation time

Also, --cache-dir=somedir doesn't work. It creates somedir, but leaves it empty and writes all the cache files to .nmlcache. Bleh.

@glx22
Copy link
Contributor

glx22 commented May 11, 2020 •
edited

Maybe just add -n or --no-cache to NML_FLAGS

Edit: and of course I forgot to refresh before commenting

FLHerne added a commit to FLHerne/nml that referenced this issue May 12, 2020
@FLHerne
Fix OpenTTD#139: Don't create cache directories if --no-cache is passed.
4a36d68
FLHerne added a commit to FLHerne/nml that referenced this issue May 12, 2020
@FLHerne
Fix OpenTTD#139: Don't create cache directories if --no-cache is passed.
efab357
FLHerne added a commit to FLHerne/nml that referenced this issue May 12, 2020
@FLHerne
Fix OpenTTD#139: Don't create cache directories if --no-cache is passed.
9f95aa2
FLHerne added a commit to FLHerne/nml that referenced this issue May 16, 2020
@FLHerne
Fix OpenTTD#139: Don't create the cache directory if --no-cache is pa…
bf2a919 
…ssed
FLHerne added a commit to FLHerne/nml that referenced this issue May 16, 2020
@FLHerne
Fix OpenTTD#139: Don't create the cache directory if --no-cache is pa…
749d017 
…ssed
@matthijskooijman
Copy link
Contributor Author

I don't actually think this is solved yet. The referenced commit makes --no-cache work again, but regression testing does not actually pass --no-cache / -n yet:

nml/regression/Makefile

Line 12 in 6817087

NML_FLAGS ?= -s -c --verbosity=1

Haven't tested yet, though.

@glx22
Copy link
Contributor

glx22 commented May 18, 2020

You're right, .nmlcache is still created when running regression tests.

@FLHerne
Copy link
Contributor

FLHerne commented May 18, 2020 •
edited

I don't actually think this is solved yet. The referenced commit makes --no-cache work again, but regression testing does not actually pass --no-cache / -n yet

It's specified in the actual nml calls for the examples: $(NMLC) $(NML_FLAGS) -n ..., because I did think of this when I originally added those to the regression testing.

I checked that no .nmlcache dirs are created in the examples.

The first pass of the tests under regression has never set -n (which was why I didn't add it to the flags), which conceivably serves some purpose by testing the cache-writing code. This creates regression/.nmlcache, which shouldn't matter as the tests aren't distributed anyway.

@glx22
Copy link
Contributor

glx22 commented May 18, 2020

Oh right, just tested and only regression/.nmlcache is created.

@matthijskooijman
Copy link
Contributor Author

Ah, good call, hadn't seen the -n in there. The regression/.nmlcache directory should indeed not be problematic, though it would still be good if make -C regression clean would clean up that directory too (for which I think passing --cache-dir explicitly, as in my suggestion in the first post, would still be good to not make any assumptions on the default cache dir location).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@matthijskooijman @glx22 @FLHerne