Set BOLOS version through env var #1
Conversation
|
@emilbayes Good contribution, thanks! Sorry for the delayed response. Could you document this arg in the readme? |
|
See my ammendments |
Co-Authored-By: emilbayes <github@tixz.dk>
| ```sh | ||
| docker run \ | ||
| -e "BOLOS_ENV=/opt/bolos-env" \ # Where to store custom compiler tooling | ||
| -e "BOLOS_SDK=/opt/bolos-sdk" \ # Where to store the BOLOS SDK itself |
There was a problem hiding this comment.
Have you tested this?
I'm pretty sure it won't work. BOLOS_SDK is set at build time as the target dir for the BOLOS SDK in the built Docker image.
If you then overwrite BOLOS_SDK at runtime it will be referencing the wrong directory and will fail to build. Same for BOLOS_ENV.
There was a problem hiding this comment.
Also, I'm not sure we need to document BOLOS_SDK or BOLOS_ENV at all. The Docker image is designed to abstract all the SDK complexity away.
Is there a benefit you see to being able to change the target dirs?
There was a problem hiding this comment.
If I got to decide I would make a new tag for each bolos version such that the toolchain and sdk version fit together. I suspect that with these very specific compilers, they are very tightly coupled (ie the custom linker scripts in bolos). But of course that leads to a higher maintenance burden on you
There was a problem hiding this comment.
Right but that's a different issue.
So there's the issue of how we set the BOLOS version, env var or Docker tag.
And then there's the issue I'm raising here that I don't think it makes sense to recommend users change the installation target directory.
You raise a good point re versioning with Docker tags, I've considered that before. Out of interest, why did you open a PR to version it with an env var if you think it should be versioned via Docker tags? 😆
There was a problem hiding this comment.
Hehe, because on one hand I thought it would be a waste to make a project that was exactly the same, but on the other hand I didn't want to have to wait for a PR to be merged on every new version :)
There was a problem hiding this comment.
Yeah, I think env var is a good trade off for now, but we can look at proper versioning with docker tags if an update stops working in the future.
If you can address my initial feedback, I think this is good to be merged.
Have you tested this?
I'm pretty sure it won't work.
BOLOS_SDKis set at build time as the target dir for the BOLOS SDK in the built Docker image.If you then overwrite
BOLOS_SDKat runtime it will be referencing the wrong directory and will fail to build. Same forBOLOS_ENV.
|
Would be great to add support for git branches (commits?) since fix for Chrome u2f is not merged to master yet and it'd be handy to use environment variables for such situations. |
|
@chicoxyzzy yeah definitely. The entire checkout arg should be an ENV variable. That way you can pass in branch/tag/commit and git will resolve it. |
No description provided.