Merge pull request #7381 from jepler/issue7380

jepler · web-flow · commit b0a635af4dc7 · 2022-12-27T11:42:24.000-06:00
Correctly handle settings.toml that ends without a newline
diff --git a/Makefile b/Makefile
@@ -90,7 +90,7 @@ clean:
 	rm -rf autoapi
 	rm -rf $(STUBDIR) $(DISTDIR) *.egg-info
 
-html: stubs
+html:
 	$(SPHINXBUILD) -b html $(ALLSPHINXOPTS) $(BUILDDIR)/html
 	@echo
 	@echo "Build finished. The HTML pages are in $(BUILDDIR)/html."
diff --git a/locale/circuitpython.pot b/locale/circuitpython.pot
@@ -65,6 +65,11 @@ msgstr ""
 msgid "%02X"
 msgstr ""
 
+#: shared-module/os/getenv.c
+#, c-format
+msgid "%S"
+msgstr ""
+
 #: shared-bindings/rgbmatrix/RGBMatrix.c
 #, c-format
 msgid ""
@@ -488,6 +493,11 @@ msgstr ""
 msgid "Already scanning for wifi networks"
 msgstr ""
 
+#: shared-module/os/getenv.c
+#, c-format
+msgid "An error occurred while retrieving '%s':\n"
+msgstr ""
+
 #: ports/stm/common-hal/audiopwmio/PWMAudioOut.c
 msgid "Another PWMAudioOut is already active"
 msgstr ""
@@ -1754,10 +1764,6 @@ msgid ""
 "constructor"
 msgstr ""
 
-#: ports/espressif/common-hal/espulp/ULP.c
-msgid "Pins 21+ not supported from ULP"
-msgstr ""
-
 #: ports/raspberrypi/common-hal/imagecapture/ParallelImageCapture.c
 msgid "Pins must be sequential"
 msgstr ""
diff --git a/shared-module/os/__init__.h b/shared-module/os/__init__.h
@@ -38,9 +38,11 @@ typedef enum {
 // Allocation free version that returns the full length of the value.
 // If it fits, the return value is 0-terminated. The passed in buffer
 // may be modified even if an error is returned.  Allocation free.
+// An error that is not 'open' or 'not found' is printed on the repl.
 os_getenv_err_t common_hal_os_getenv_str(const char *key, char *value, size_t value_len);
 
 // Returns GETENV_OK and sets value to the read value.  Returns
 // GETENV_ERR_... if the value was not numeric. allocation-free.
 // If any error code is returned, value is guaranteed not modified
+// An error that is not 'open' or 'not found' is printed on the repl.
 os_getenv_err_t common_hal_os_getenv_int(const char *key, mp_int_t *value);
diff --git a/shared-module/os/getenv.c b/shared-module/os/getenv.c
@@ -28,6 +28,7 @@
 // tested in the unix "coverage" build, without bringing in "our" os module
 
 #include <stdlib.h>
+#include <stdarg.h>
 #include <string.h>
 
 #include "shared-bindings/os/__init__.h"
@@ -36,6 +37,7 @@
 #include "py/gc.h"
 #include "py/misc.h"
 #include "py/mpstate.h"
+#include "py/mpprint.h"
 #include "py/objstr.h"
 #include "py/parsenum.h"
 #include "py/runtime.h"
@@ -199,6 +201,7 @@ STATIC os_getenv_err_t read_string_value(file_arg *active_file, vstr_t *buf) {
                     case '#':
                         next_line(active_file);
                         MP_FALLTHROUGH;
+                    case 0:
                     case '\n':
                         return GETENV_OK;
                     default:
@@ -255,7 +258,6 @@ STATIC os_getenv_err_t read_bare_value(file_arg *active_file, vstr_t *buf, int f
     while (true) {
         switch (character) {
             case 0:
-                return GETENV_ERR_UNEXPECTED | character;
             case '\n':
                 return GETENV_OK;
             case '#':
@@ -311,42 +313,70 @@ STATIC os_getenv_err_t os_getenv_buf_terminated(const char *key, char *value, si
     return result;
 }
 
-os_getenv_err_t common_hal_os_getenv_str(const char *key, char *value, size_t value_len) {
-    bool quoted;
-    os_getenv_err_t result = os_getenv_buf_terminated(key, value, value_len, &quoted);
-    if (result == GETENV_OK && !quoted) {
-        result = GETENV_ERR_UNEXPECTED | value[0];
-    }
-    return result;
+STATIC void print_dont_raise(const mp_obj_type_t *exc_type, const compressed_string_t *fmt, ...) {
+    va_list argptr;
+    va_start(argptr,fmt);
+    mp_vcprintf(&mp_plat_print, fmt, argptr);
+    mp_printf(&mp_plat_print, "\n");
+    va_end(argptr);
 }
 
-STATIC void throw_getenv_error(os_getenv_err_t error) {
+STATIC void handle_getenv_error(os_getenv_err_t error, void (*handle)(const mp_obj_type_t *exc_type, const compressed_string_t *fmt, ...)) {
     if (error == GETENV_OK) {
         return;
     }
     if (error & GETENV_ERR_UNEXPECTED) {
         byte character = (error & 0xff);
-        mp_print_t print;
+        char buf[8];
         vstr_t vstr;
-        vstr_init_print(&vstr, 8 + 4 + 1, &print);
+        vstr_init_fixed_buf(&vstr, sizeof(buf), buf);
+        mp_print_t print = { .data = &vstr, .print_strn = (mp_print_strn_t)vstr_add_strn };
+
         if (character) {
             mp_str_print_quoted(&print, &character, 1, true);
         } else {
             mp_str_print_quoted(&print, (byte *)"EOF", 3, true);
         }
-        mp_raise_ValueError_varg(translate("Invalid byte %.*s"),
-            vstr.len, vstr.buf);
+        handle(&mp_type_ValueError, translate("Invalid byte %.*s"), vstr.len, vstr.buf);
+    } else {
+        switch (error) {
+            case GETENV_ERR_OPEN:
+                handle(&mp_type_ValueError, translate("%S"), translate("File not found"));
+                break;
+            case GETENV_ERR_UNICODE:
+                handle(&mp_type_ValueError, translate("%S"), translate("Invalid unicode escape"));
+                break;
+            case GETENV_ERR_NOT_FOUND:
+                handle(&mp_type_ValueError, translate("%S"), translate("Key not found"));
+                break;
+            default:
+                handle(&mp_type_RuntimeError, translate("%S"), translate("Internal error"));
+                break;
+        }
+    }
+}
+
+STATIC void common_hal_os_getenv_showerr(const char *key, os_getenv_err_t result) {
+    if (result != GETENV_OK && result != GETENV_ERR_OPEN && result != GETENV_ERR_NOT_FOUND) {
+        mp_cprintf(&mp_plat_print, translate("An error occurred while retrieving '%s':\n"), key);
+        handle_getenv_error(result, print_dont_raise);
     }
-    switch (error) {
-        case GETENV_ERR_OPEN:
-            mp_raise_ValueError(translate("File not found"));
-        case GETENV_ERR_UNICODE:
-            mp_raise_ValueError(translate("Invalid unicode escape"));
-        case GETENV_ERR_NOT_FOUND:
-            mp_raise_ValueError(translate("Key not found"));
-        default:
-            mp_raise_RuntimeError(translate("Internal error"));
+}
+
+STATIC
+os_getenv_err_t common_hal_os_getenv_str_inner(const char *key, char *value, size_t value_len) {
+    bool quoted;
+    os_getenv_err_t result = os_getenv_buf_terminated(key, value, value_len, &quoted);
+    if (result == GETENV_OK && !quoted) {
+        result = GETENV_ERR_UNEXPECTED | value[0];
     }
+    return result;
+}
+
+os_getenv_err_t common_hal_os_getenv_str(const char *key, char *value, size_t value_len) {
+    os_getenv_err_t result = common_hal_os_getenv_str_inner(key, value, value_len);
+    common_hal_os_getenv_showerr(key, result);
+    return result;
 }
 
 mp_obj_t common_hal_os_getenv_path(const char *path, const char *key, mp_obj_t default_) {
@@ -358,7 +388,7 @@ mp_obj_t common_hal_os_getenv_path(const char *path, const char *key, mp_obj_t d
     if (result == GETENV_ERR_NOT_FOUND || result == GETENV_ERR_OPEN) {
         return default_;
     }
-    throw_getenv_error(result);
+    handle_getenv_error(result, mp_raise_msg_varg);
 
     if (quoted) {
         return mp_obj_new_str_from_vstr(&mp_type_str, &buf);
@@ -371,7 +401,7 @@ mp_obj_t common_hal_os_getenv(const char *key, mp_obj_t default_) {
     return common_hal_os_getenv_path(GETENV_PATH, key, default_);
 }
 
-os_getenv_err_t common_hal_os_getenv_int(const char *key, mp_int_t *value) {
+STATIC os_getenv_err_t common_hal_os_getenv_int_inner(const char *key, mp_int_t *value) {
     char buf[16];
     bool quoted;
     os_getenv_err_t result = os_getenv_buf_terminated(key, buf, sizeof(buf), &quoted);
@@ -389,3 +419,9 @@ os_getenv_err_t common_hal_os_getenv_int(const char *key, mp_int_t *value) {
     *value = (mp_int_t)num;
     return GETENV_OK;
 }
+
+os_getenv_err_t common_hal_os_getenv_int(const char *key, mp_int_t *value) {
+    os_getenv_err_t result = common_hal_os_getenv_int_inner(key, value);
+    common_hal_os_getenv_showerr(key, result);
+    return result;
+}
diff --git a/tests/circuitpython/getenv.py b/tests/circuitpython/getenv.py
@@ -35,7 +35,7 @@ def ioctl(self, op, arg):
 uos.VfsFat.mkfs(bdev)
 uos.mount(uos.VfsFat(bdev), "/")
 
-content_good = """
+content_good = b"""
 # comment
 key0 = "hello world"
 key1 = 7
@@ -54,15 +54,15 @@ def ioctl(self, op, arg):
 """
 
 content_bad = [
-    'key = "\n',
-    'key = """\n',
-    "key =\n",
-    'key="',
+    b'key = "\n',
+    b'key = """\n',
+    b"key =\n",
+    b'key="',
 ]
 
 
 def run_test(key, content):
-    with open("/settings.toml", "w") as f:
+    with open("/settings.toml", "wb") as f:
         f.write(content)
 
     try:
@@ -75,5 +75,12 @@ def run_test(key, content):
 for i in range(13):
     run_test(f"key{i}", content_good)
 
+content_good = content_good.replace(b"\n", b"\r\n")
+for i in range(13):
+    run_test(f"key{i}", content_good)
+
+# Test value without trailing newline
+run_test(f"noeol", b"noeol=3")
+
 for content in content_bad:
     run_test("key", content)
diff --git a/tests/circuitpython/getenv.py.exp b/tests/circuitpython/getenv.py.exp
@@ -11,6 +11,20 @@ key9 'hello comment'
 key10 127
 key11 0
 key12 None
+key0 'hello world'
+key1 7
+key2 Invalid byte '\n'
+key3 'Áx'
+key4 'Áx'
+key5 Invalid byte '\\'
+key6 '\t\r\x08'
+key7 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'
+key8 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'
+key9 'hello comment'
+key10 127
+key11 0
+key12 None
+noeol 3
 key Invalid byte '\n'
 key Invalid byte '"'
 key invalid syntax for integer with base 10: ''
