Automate build a bit more #10
Conversation
|
Any interest in this? It makes the build process one-step ( |
|
I like the automatic tiny-AES patching! Less sure of the virtualenv part (some people use conda, some use pipenv, some people are cowboy and install system-wide...). Semi-related, I hope to replace the python-fido2 fork with an extension (or upstream addition), so we can publish a wheel on PyPI instead of tracking upstream changes. @conorpp ? |
|
Yeah I think the first commit is good. Later on, I think I'd like to do something else about configuring the AES library. Letting the Makefile patch a header file seems a bit hacky. Maybe we should just make a copy of the tiny-AES header file? |
|
The venv stuff is just.. there. You don't have to use it, but it's handy and portable such that if all you have is python3 and this repo then it will manage everything for you. I don't think it does anything that precludes anyone from using conda or pipenv or system wide or whatever, it's just a way to get a known-good install. |
Makefile
Outdated
| @@ -64,3 +83,9 @@ uECC.o: ./crypto/micro-ecc/uECC.c | |||
|
|
|||
| clean: | |||
| rm -f *.o main.exe main $(obj) | |||
| for f in crypto/tiny-AES-c/Makefile tinycbor/Makefile ; do \ | |||
| if [ -f "$$f" ]; then \ | |||
| (cd `dirname $$f` ; git co -- .) ;\ | |||
There was a problem hiding this comment.
IMO, this should read
git checkout -- .
There's people out there not having set this alias.
Makefile
Outdated
| # python virtualenv | ||
|
|
||
| venv: | ||
| python3 -m venv venv |
There was a problem hiding this comment.
I'm not sure about availability across Linux distributions here. Better check for the package/binary and if missing bail out w/ some useful message a la
ERR: Sorry, no python virtualenv found. Please consider installing via
sudo apt install python-virtualenv virtualenv
There was a problem hiding this comment.
Interesting. On ubuntu 18.04.1 I get something more verbose:
pj@trireme:~/src/solo$ make venv
python3 -m venv venv
The virtual environment was not created successfully because ensurepip is not
available. On Debian/Ubuntu systems, you need to install the python3-venv
package using the following command.
apt-get install python3-venv
You may need to use sudo with that command. After installing the python3-venv
package, recreate your virtual environment.
Failing command: ['/home/pj/src/solo/venv/bin/python3', '-Im', 'ensurepip', '--upgrade', '--default-pip']
Makefile:88: recipe for target 'venv' failed
make: *** [venv] Error 1
I was presuming that it was included everywhere by default since it now ships as part of python, but I guess I'm wrong. I'l switch to using virtualenv and requesting it if not found in the $PATH.
| clean: | ||
| rm -f *.o main.exe main $(obj) | ||
| for f in crypto/tiny-AES-c/Makefile tinycbor/Makefile ; do \ | ||
| if [ -f "$$f" ]; then \ | ||
| (cd `dirname $$f` ; git co -- .) ;\ | ||
| fi ;\ | ||
| done | ||
| rm -rf venv |
There was a problem hiding this comment.
Unconditionally calling rm -rf in a user script always raises my attention (goosebumps ahead!). At least, we should
rm -r ./venv/* && rm -rf ./venv/.git/ && rmdir -p ./venv
(Revise ./venv/ content and adapt to requirements)
YMMV.
Personal pref: set PATH @top of Makefile and explicitely call binaries:
/bin/rm foo
There was a problem hiding this comment.
We create the venv above, and the rm -rf has an explicit filename with no wildcards on it. The venv is a build-product, like an OBJDIR or something. It doesn't have a .git directory in it, and if it did, I'm not sure how it'd be
There was a problem hiding this comment.
Yes, it's not so clear how one could improve on this one. Except a fully qualified path is always a good idea, security-wise.
| rm -rf venv | |
| /bin/rm -rf venv |
Anyhow, it is just the call to rm -rf in a script that's raising my attention. But as you mentioned it, there is no variable involved which could eval to the empty element ("") and no wildcard either. So, with a fq-path, it seems reasonable enough.
Makefile
Outdated
| crypto/tiny-AES-c/aes.o: | ||
| if ! grep "^#define AES256" crypto/tiny-AES-c/aes.h ; then \ | ||
| echo "Fixing crypto/tiny-AES-c/aes.h" ;\ | ||
| sed -i 's/^#define AES1\/\/#define AES1; s/^\/*#define AES256/#define AES256/' crypto/tiny-AES-c/aes.h ;\ |
There was a problem hiding this comment.
This isn't exactly what I would call portable..
There was a problem hiding this comment.
It uses bash and grep and sed... I'm open to fixing it to use more portable tools if you'll specify them.
There was a problem hiding this comment.
Actually, further investigation shows you shouldn't've been doing it this way anyway. You can just add -DAES256=1 to CFLAGS and it will do the same thing. Updated version coming.
Makefile
Outdated
| $(name): $(obj) | ||
|
|
||
| crypto/tiny-AES-c/aes.o: | ||
| if ! grep "^#define AES256" crypto/tiny-AES-c/aes.h ; then \ |
There was a problem hiding this comment.
I'd silence grep as we tell the user anyway if we are to patch:
if ! grep -q "^#define AES256" crypto/tiny-AES-c/aes.h 2>/dev/null ; then \
There was a problem hiding this comment.
I'm 👍 on the -q , but testing with both true and false versions it produces no output, so I think the 2>/dev/null is unnecessary. If there's some other failure (like crypto/tiny-AES-c/aes.h not being found, for instance) then there should be an error surfaced. That file should definitely exist, however, due to the implicit dependency (*.o depends on *.h) which makes the submodules get updated on line 32.
|
All looks fine to me. Thanks!! Nice catch on the |
Alpha updates
No description provided.