marcelstoer / docker-nodemcu-build

Docker image to build NodeMCU firmware for the ESP8266 on your machine
https://hub.docker.com/r/marcelstoer/nodemcu-build/
MIT License
129 stars 63 forks source link

IMAGE_NAME Option Anomaly #45

Closed jmd13391 closed 5 years ago

jmd13391 commented 5 years ago

RE: https://hub.docker.com/r/marcelstoer/nodemcu-build/

image

...using the IMAGE_NAME option in the "Create the Firmware" step with user_config.h "#define LUA_NUMBER_INTEGRAL" enabled:

docker run --rm -it -e "IMAGE_NAME=firmware" -v "//c/Users/Joe/Projects/NodeMCU/nodemcu-firmware":/opt/nodemcu-firmware marcelstoer/nodemcu-build build

...produces the expected result file name:

nodemcu_integer_firmware.bin

=-=

...using the IMAGE_NAME option in the "Create an LFS Image" step with user_config.h "#define LUA_NUMBER_INTEGRAL" enabled:

docker run --rm -ti -e "IMAGE_NAME=firmware" -v "//c/Users/Joe//Projects/NodeMCU/nodemcu-firmware":/opt/nodemcu-firmware -v "//c/Users/Joe/Projects/NodeMCU/user-apps":/opt/lua marcelstoer/nodemcu-build lfs-image

...produces an unexpected result file name:

firmware_int.img

Based on the documentation for the IMAGE_NAME option, the expected result file name should have:

nodemcu_integer_firmware.img

=-=

marcelstoer commented 5 years ago

@HHHartmann do you remember if you explicitly tested this when you added the LFS features? I'm sure I didn't, sorry.

HHHartmann commented 5 years ago

It seems as if I also missed it. I'm currently quite busy but hopefully can have a look soon.

marcelstoer commented 5 years ago

This actually ain't a bug I just realized. Sorry, I only gave this report a cursory look the first time.

All the options are for the firmware build command (docker ... build). That's why they're documented in a sub chapter of "Run this image with Docker to create the firmware".

The LFS chapter in "Run this image with Docker to create an LFS image" doesn't document any options. Currently there are none.

However, I see no reason why this couldn't be supported as Joe suggested.

jmd13391 commented 5 years ago

@marcelstoer, There's more to this... :-(

For the LFS image step, if you do not specify IMAGE_NAME, the result file is LFS_2018-10-03 1006_int.img. (note the odd character between hh & mm). I would suggest you consider the following file name format as default for BOTH build sections...

{nodemcu|lfs}_{float|integer}_{branch}_{yyyymmdd_hhmm}.{bin|img}

...the key points of my recommendation being the use of all lowercase and the removal of spaces, special characters and dashes** from the default file name.

** Working on the Windows 10 platform with ESPlorer and/or NodeMCU-Tool, I encountered intermittent failure oddness when trying to node.flashreload() image files that contained dashes. Simply removing the dashes eliminated the failures. I will attempt to capture & report the issue next time I run across it.

-Joe

HHHartmann commented 5 years ago

Hi, sorry for the delay but I was in London sightseeing which kept me really busy.

Obviously the IMAGE_NAME option is supported but not documented for lfs images. Surely the resulting filename can be argued and there seems to be a break in using int or integer in our ecosystem. For the firmware images the term "integer" is used, for the luac.cross compilers "int" is used. But I agree, that filenames of the artifacts created here should be unified.

As for the branch part I am not so sure. For the firmware bin files it might make sense, but the wouldn't the exact commit be needed here (also)? For LFS images you mean the branch of the Lua files or the nodemcu-firmware?

As for the odd character at the default filename, that seems to be related to the colon (:) which is an invalid character for windows filenames of cause.

HHHartmann commented 5 years ago

Ups, just noticed that the branch is already part of the filename for firmware bin files. But still what branch to use for the LFS images?

jmd13391 commented 5 years ago

@HHHartmann , It would be helpful for application developers such as myself if the LFS img file build step associated the app img bundle with a particular the firmware build branch and type (int/float) it was compiled under. This is especially helpful when like me, you have several variants of the app bundle floating around at the same time. In my case, production app releases are mostly bundled with master firmware builds (for obvious stability reasons). However, there are certainly exceptions... especially in the past few months with my production roll-outs not being able to wait until a critical show-stopper firmware PR lands in master (ex: #2494)

If there is a way to extract the firmware branch and type identifier at LFS img compile time and apply that detail to the img name, that would be preferred (and it would go a long way towards having a standard file naming convention across the build steps).

HHHartmann commented 5 years ago

@jmd13391 the view on an LFS image might mostly be different. While compiled under a certain nodemcu-firmware branch/version it is the same all the time unless the luac.cross has changed. Which only happens rarely hopefully. So a versioning of the luac.cross file format would be needed.

@TerryE what do you think about adding the version information to the luac.cross version command?

BTW it might not really help to just add the branch. In your scenario it would be "master" all the time anyways if I understood you correctly. Adding the release would help better. @marcelstoer What do you think about that? Can we access that from git command line? (I know, my git knowledge is poor) Maybe the release if on "master", else the branch.

jmd13391 commented 5 years ago

@HHHartmann Sigh... sounds like a lot of work. I guess it would be easier for me to manually rename the img file as I have been doing all along.

In your scenario it would be "master" all the time

...definitely desirable. Unfortunately, not the case when you have multiple ESP8266's on the bench, developing for multiple products. Living on the NodeMCU firmware "edge" (partly because product roll-out cannot wait months for a critical path function or feature or fix to land in master) requires me to sometimes develop apps that will only work with the dev firmware branch. Once the apps are compiled into an img file, that img file needs to be renamed so as to identify the firmware flavor associated with it.

HHHartmann commented 5 years ago

Just because it sounds like work does not mean it can't be done. If we find a solution good for most scenarios it might be worth it. Having a version info for luac.cross is just a matter of printing it as the img files have a versioning already. But coupling the default filename of the LFS img to the firmware version the luac.cross is built with makes sense in your case but not generally i think, as people might get their firmware separately from the LFS image. (use the docker only for LFS and get the firmware from online service) It would also suggest a dependency between LGS image and firmware version that somewhat exists but not that tight.

HHHartmann commented 5 years ago

Created PR #47 to unify filenames @jmd13391 please feel free to add an issue to get the luac.cross version in the filename of the LFS image. I will see if I get it done if the owner/collaborators agree.

marcelstoer commented 5 years ago

But coupling the default filename of the LFS img to the firmware version the luac.cross is built with makes sense in your case but not generally i think

👍