search

mit-dci / opencbdc-tx

A transaction processor for a hypothetical, general-purpose, central bank digital currency
Other
896 stars 198 forks source link

CI/CD breakage #283

Closed HalosGhost closed 1 month ago

HalosGhost commented 1 month ago

@rockett-m, it looks like the docker merge CI/CD step is failing from the merge of #265. It's not spurious (rerunning the job failed as well). Help resolving would be appreciated.

maurermi commented 1 month ago

@rockett-m, it looks like the docker merge CI/CD step is failing from the merge of #265. It's not spurious (rerunning the job failed as well). Help resolving would be appreciated.

It looks like the failure occurs when running install-build-tools.sh in docker merge, and it appears that the docker merge job uses sh instead of bash. I think the problem is now we are calling source in this script which causes the error since source is not supported by sh.

(Sending this from my phone, will verify further when able)

rockett-m commented 1 month ago

Just got online. Michael, that sounds logical.

Would it be ok to add shell: bash to the parameters for the docker merge CI/CD step? inside .github/workflows/docker-merge.yml

defaults:
  run:
    shell: bash

I'll set that preference and try with act -j docker-build --container-architecture linux/amd64 to see if that runs the source correctly and passes.

maurermi commented 1 month ago

I am not sure of an argument for using sh instead of bash, so I do not see a problem with this (@HalosGhost you may have a more nuanced opinion here)

HalosGhost commented 1 month ago

Out of a preference for portability, I'd probably be happiest if we completely ripped out bash and relied purely on POSIX sh, but I don't think that's really a reasonable standard given how constrained our support matrix is right now anyway. Plus, bash is already pretty intertwined as a dep.

I'm entirely fine with our workflows requiring bash. Having said that, the POSIX spelling of source is ., and we could reasonably do that instead (as bash is POSIX-compatible, . works in it as well—same for zsh and any other POSIX-compatible shell).

rockett-m commented 1 month ago

That's a good idea with the '.' swapped in. I'll give it a go.

In our ci.yml we have the shell defaults set to bash so that's why I suggested it initially.

rockett-m commented 1 month ago

I think the issue could be related to the Dockerfile where the venv file should be included so it can be found by the CI/CD docker-merge...will continue testing. Note - that alone does not resolve the issue.

COPY scripts/install-build-tools.sh /opt/tx-processor/scripts/install-build-tools.sh
COPY scripts/setup-dependencies.sh /opt/tx-processor/scripts/setup-dependencies.sh
COPY scripts/activate-venv.sh /opt/tx-processor/scripts/activate-venv.sh

traced from here:

**| 249.5 scripts/install-build-tools.sh: line 89: scripts/activate-venv.sh: No such file or directory**
| ------
| Dockerfile:23
| --------------------
|   21 |     WORKDIR /opt/tx-processor
|   22 |     
|   23 | >>> RUN /usr/bin/env bash -c scripts/install-build-tools.sh
|   24 |     RUN scripts/setup-dependencies.sh
|   25 |     
| --------------------