chmorgan / libesphttpd

Libesphttpd - web server for ESP8266 / ESP32
Mozilla Public License 2.0
128 stars 45 forks source link

Missing dependency on HTML files. #26

Closed itronics closed 6 years ago

itronics commented 6 years ago

Changes in embedded html/js files are ignored by esp-idf make system, which results in incomplete OTA update files. Attached patch fixes this problem. patch.txt

chmorgan commented 6 years ago

Hi @itronics I can see how this could fix a linker issue but I'm not sure how to reproduce it here, do you have any tips on how to reproduce the issue this fixes?

The existing issue of changing a web file not resulting in a rebuild of the embedded filesystem is still present.

itronics commented 6 years ago

Hi, You can reproduce the issue on esphttpd-freertos project files. Changing e.g. wifi/index.html or websocket/index.html will not trigger rebuilding of liblibesphttpd library, so the result bin file will not contain the changes (changes in root html dir i.e. style.css or index.tpl seems to be detected properly).

Unfortunately the patch I've attached above is not enough to fix the issue (html files are not repacked before linking).

chmorgan commented 6 years ago

Hi @itronics yeah that's what I'm seeing. I just don't know the proper make-fu to cause a rebuild when any of the html files change. I'd probably avoid including the change until we can fix the entire issue as I'm not able to see any affect of this change here after changing files in the HTML_DIR.

tidklaas commented 6 years ago

You could use find to build a list of all files and directories in the HTML dir and make the target webpages.espfs depend on all of them:

index 359aa5e..2f90d56 100644
--- a/component.mk
+++ b/component.mk
@@ -16,6 +16,7 @@ COMPONENT_ADD_LDFLAGS := -lwebpages-espfs -llibesphttpd
 COMPONENT_EXTRA_CLEAN := mkespfsimage/*

 HTMLDIR := $(subst ",,$(CONFIG_ESPHTTPD_HTMLDIR))
+HTMLFILES := $(shell find $(PROJECT_PATH)/$(HTMLDIR))

 JS_MINIFY_TOOL ?= uglifyjs
 CFLAGS += -DFREERTOS -DESPFS_HEATSHRINK
@@ -30,7 +31,7 @@ liblibesphttpd.a: libwebpages-espfs.a

 # mkespfsimage will compress html, css, svg and js files with gzip by default if enabled
 # override with -g cmdline parameter
-webpages.espfs: $(PROJECT_PATH)/$(HTMLDIR) mkespfsimage/mkespfsimage
+webpages.espfs: $(HTMLFILES) mkespfsimage/mkespfsimage
 ifeq ("$(CONFIG_ESPHTTPD_USEUGLIFYJS)","y")
        echo "Using uglifyjs"
        rm -rf html_compressed;
chmorgan commented 6 years ago

@tidklaas wow this works! I expected it to be way more complicated. Thanks for sending that over. I'll try to get it in in the near future. @itronics do you want to test it out as well to see if this resolves the issue for you? Wouldn't hurt to have a second test run.

tidklaas commented 6 years ago

@chmorgan there is a little caveat, though. The build will break if there are any file or directory names containing whitespace. This can easily be fixed on proper, UN*X-like systems using sed: HTMLFILES := $(shell find $(PROJECT_PATH)/$(HTMLDIR) | sed -E 's/([[:space:]])/\\\1/g')

I do not know if this will work under Windows because of the use of '\' as the directory separator. Alternatively, one could simply add a rule: Thou shalt not create entries with blanks, for they are abomination and anger the MAKE.

itronics commented 6 years ago

Yes, the patch works like a charm. Thank you both for fast response. The bug was not lethal, but very annoying :).

chmorgan commented 6 years ago

@tidklaas what is that sed doing? Replacing spaces with??

tidklaas commented 6 years ago

@chmorgan all whitespace characters are escaped by prepending a backslash. So, for example, 'foo bar' becomes 'foo\ bar' and 'bar baz' becomes 'bar\ \ baz'.

chmorgan commented 6 years ago

@tidklaas, @itronics changes pushed to master. Thank you @tidklaas!