Merge with dev & update to a docker build environment #2394
Conversation
```diff diff .gitignore .dockerignore 10,13d9 < credentials.h < node_modules < code/utils < custom.h ´´´
…v-merge-copyrights
this is based on the copyrights/espurna:docker-build branch * moving to `debian:stable-slim` * moving to specifically python3 * fixing the way that pip is used to ensure that we're getting the copy we need for the target packages * removing the UID/GID so that it can just be used with `sudo`, can worry about adding the back in when an approved container is built to support vscode (https://code.visualstudio.com/docs/remote/containers)
| Cloning into '/root/.platformio/.cache/tmp/pkg-installing-rwukc302'... | ||
| Cloning into '/root/.platformio/.cache/tmp/pkg-installing-20osjkr2'... |
There was a problem hiding this comment.
I think previous version meant to use userid map here?
There was a problem hiding this comment.
previously, but there was questions on if everyone would have a valid userid that started at 1000 or similar, figured it was just easier to drop it than deal with it as most times people just sudo the docker anyway (for good or bad).
There was a problem hiding this comment.
If this is just an example. Build sends things as root already, so the file needs some tweaking too, i.e. docker run --id is not enough.
There was a problem hiding this comment.
I don't know that I understand - are you wanting to wire back in the previous ID stuff that wasn't working / might not be compatible or you are looking for something else?
There was a problem hiding this comment.
Previous ID was fixed, true, I'm just saying that in case docker run --id=$(id -u) is used, it will fetch build tools all over again, since PIO installs stuff in the $HOME/.platformio
Maybe we want a comment in the readme with --build-arg UID=$(id -u) --build-arg GID=$(id -g)?
https://docs.docker.com/engine/reference/commandline/build/#set-build-time-variables---build-arg
| RUN apt-get update \ | ||
| && apt-get install -y --no-install-recommends \ | ||
| python3-minimal \ | ||
| python3-pip \ | ||
| python3-setuptools \ | ||
| ca-certificates \ | ||
| nodejs \ | ||
| npm \ | ||
| git | ||
|
|
||
| # NOTE: there is currently a compability warning with the target versions of Node vs. NPM - using latest until something is identified as the target version | ||
| RUN rm -rf /var/lib/apt/lists/* \ | ||
| && npm install npm@latest -g | ||
|
|
||
| # NOTE: Python module for pip requires upgrade adhead of PIO install to work around defects | ||
| RUN python3 -m pip install -U pip |
There was a problem hiding this comment.
Nit / rant / OT (?)
Perhaps I was living in a bubble so far, since I used very little of debian-like stuff outside of desktop / real servers. I guess this works for now, but I just tried this one:
FROM fedora:latest
RUN dnf -y install nodejs python python-pip git && dnf clean all
RUN pip install platformioSince we want bleeding-edge sw anyways... Feels like a waste doing all of this with stable debian :)
(...and gyp also pulls python2, we need to wait until march-april for the next release including it, or pull from testing repos... I wonder if things like pyenv + nodeenv would do a better job instead)
There was a problem hiding this comment.
doesn't really matter to me - just happened to be what was there and it looked like the repo needed more than just the python or node images.
i will say that i'd still recommend explicitness to avoid any random breakage (python3 vs. python :) )
| RUN npm install --only=dev && \ | ||
| platformio run --target clean |
There was a problem hiding this comment.
I missed this part. We also want the libs installed here? Check out the scripts/pio_pre.py, ESPURNA_PIO_SHARED_LIBRARIES env var and the platformio.ini config entry. Since PIO4 it does install libraries per-environment, so you end up with .pio/libdeps/sonoff-basic, .pio/libdeps/sonoff-pow and etc. However, since docker makes a snapshot of the espurna version and lib_deps is shared between envs, it would make sense to place libs there.
This can be a separate target for the platformio, check out https://docs.platformio.org/en/latest/projectconf/advanced_scripting.html#custom-targets
(both platform tools and libraries in the same step)
It's not on by default though (and I'll need to remember why not...) and needs that env flag to install libs there.
There was a problem hiding this comment.
Took me a bit of digging on this to try and understand what you're looking for and I'm still not certain I understand what change your asking for here (probably because I'm not as familiar with the build environment setup). I think it's as simple as calling a platformio lib install ... but that does significantly increase the build time of the docker image, especially if you really only wanted it to build one target so you didn't have to setup a ton of environments (a couple of minutes to 20+ minutes). My original use case for this was far more for the person that had 1 target to build as opposed to all of the targets to build, given that if your run with a single target, it still installs the associated libraries that you need to generate a firmware image.
There was a problem hiding this comment.
Not quite, the idea is that libs would be fetched just once into this dir:
Lines 169 to 170 in ac6f0b7
Historically, PIO used a common library directory as .pio/libdeps/ until PIO4, which is what this essentially does try to simulate. No need to call each $env, just something that will fetch the libraries + build tools once.
- platform == platform_latest - mcspr/toolchain-xtensa via platformio/platformio-core#3612 Yet, this does not avoid useless warnings that platform does not specify platformio/* prefix in the package spec :( It should be available in the next espressif8266 version: platformio/platform-espressif8266@0859336
Already using the "latest" stable release, no need to split things (at least for now)
- provide a generic way to read default status of the pin after boot allows us to use switch as both on and off, independent of the position on boot - add BUTTON_DEFAULT_LOW & BUTTON_DEFAULT_BOOT (now it's not just BUTTON_DEFAULT_HIGH) wiki described raw numbers for some reason :/ this will break that - clean-up 'constexpr const' and 'const' function args, both are redundant - clean-up debounce event member defaults, put things in the header
Specifically, this will break RFM69_SUPPORT on case insensitive filesystems due to the RFM69.cpp from the RFM69 lib being ignored when adding ESPurna's rfm69.cpp
```diff diff .gitignore .dockerignore 10,13d9 < credentials.h < node_modules < code/utils < custom.h ´´´
this is based on the copyrights/espurna:docker-build branch * moving to `debian:stable-slim` * moving to specifically python3 * fixing the way that pip is used to ensure that we're getting the copy we need for the target packages * removing the UID/GID so that it can just be used with `sudo`, can worry about adding the back in when an approved container is built to support vscode (https://code.visualstudio.com/docs/remote/containers)
fixing silly merge conflict headers that got left over
|
I think something broke in the merge :/ It should not list changes from dev |
|
abandoning pr |
I was hoping for a docker build and it looks like it never got moved in. All credit to @copyrights for their work, I'm just coming by to try and get things wired back into place and current with
devThis should allow you to create a docker image like in #1754, which seems to have gone stale and I can't see why the code quality doesn't pass (hopefully this PR will tell me and I can fix it up).