Open proppy opened 2 years ago
@proppy, since you are converting the container action to a composite action, you might allow users to provide the image as an argument and then execute fusesoc inside. See, for instance, https://github.com/VUnit/vunit_action/blob/master/action.yml. Note that in your current approach, the environment where fusesoc is executed is the "host" (the virtual environment provided by GitHub). It's a tradeoff, either preinstalling fusesoc or doing it in each execution.
Unfortunately, there are syntax limitations in GitHub Actions that prevent using parameters for Container Steps (see https://github.com/pyTooling/Actions/#context, https://github.com/github/feedback/discussions/9049 and https://github.com/github/feedback/discussions/9053) or Containers Actions (see https://github.com/github/feedback/discussions/9153). Therefore, your proposal to use containers through edalize's launcher might be desirable to work around those limitations.
I would suggest to add an input to your action which allows users to provide a dictionary (or a configuration file) to optionally override the hardcoded containers in el_docker. That would allow users to optionally customise the environment used for each tool, without requiring further modifications to the Action.
Another detail that might have gone unnoticed is that this Action provides an alternative to el_docker already. See https://github.com/librecores/ci-fusesoc-action/blob/main/requirements.txt#L3 and https://github.com/librecores/eda-container-wrapper. It might be interesting to evaluate providing eda-container-wrapper by default with edalize, in replacement of el_docker.
@umarcor thanks for the pointers as usual!
@proppy, since you are converting the container action to a composite action, you might allow users to provide the image as an argument and then execute fusesoc inside.
I think it would be nice to also have a variant that allow developer to run the fusesoc step in their own image (if they already have one that bundle their dependencies).
Note that in your current approach, the environment where fusesoc is executed is the "host" (the virtual environment provided by GitHub).
Sorry if that was phrased poorly, but that was intentional, I was trying to getci-fusesoc-action
to run against against the main "host" environment, so that generators execution (which is not containerized) can leverage dependencies and side effect from previous steps of the same workflow.
I would suggest to add an input to your action which allows users to provide a dictionary (or a configuration file) to optionally override the hardcoded containers in el_docker.
shouldn't that be a feature of fusesoc
though? the same way you override tool options, you might want to be able to specify a given flavor
of the tool to run for your image. /cc @olofk
Another detail that might have gone unnoticed is that this Action provides an alternative to el_docker already.
I did notice that, but I maybe incorrectly assumed that that alternative predated el_docker
. So I'd welcome a bit of history/context here if anybody can share it.
Currently
ci-fusesoc-action
uses container based action (https://docs.github.com/en/actions/creating-actions/creating-a-docker-container-action) to runfusesoc
which makes easy to pull in fusesoc + all the required dependencies into the workflow environment.But sometime projects like https://github.com/GuzTech/misato, leverage additional
generators
(in misato's case:amaranth
) that require to have their dependencies installed prior to running fusesoc, see: https://github.com/GuzTech/misato/blob/main/.github/workflows/openlane.yml#L19This makes it challenging to compose with additional workflow steps, because the (contained)
ci-fusesoc-action
python environment into whichfusesoc
runs is different from the main workflow's one and wouldn't have the python package installed from previoussteps
, see https://github.com/proppy/misato/blob/f81bdeb1592000aaf70e7ed9af5071fe5cbc1136/.github/workflows/openlane.yml#L15 which currently failes with aModuleNotFoundError: No module named \'amaranth\'
error: https://github.com/proppy/misato/runs/4897990390?check_suite_focus=trueSince
fusesoc
thru https://github.com/olofk/edalize/blob/master/scripts/el_docker is already capable of launching tools packaged as container, one possible way to address this would be to provide a composite variant of the action (https://docs.github.com/en/actions/creating-actions/creating-a-composite-action), and delegate the container management tofusesoc
itself if the correspondingtool
s requires it.I made a proof of concept here https://github.com/proppy/ci-fusesoc-action/blob/main/action.yml which seems to work https://github.com/proppy/misato/runs/4899664576?check_suite_focus=true but I'm unsure on the best way to submit it for inclusion in the
ci-fusesoc-action
project, should we have it as a different actions file, or live in a different branch, or be a different project altogether?Looking forward @wallento and @olofk guidance on this!