Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci-fusesoc-action should provide a variant that allow developers to pre-install generators dependencies #1

Open
proppy opened this issue Jan 21, 2022 · 3 comments

Comments

@proppy
Copy link

proppy commented Jan 21, 2022

Currently ci-fusesoc-action uses container based action (https://docs.github.com/en/actions/creating-actions/creating-a-docker-container-action) to run fusesoc 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#L19

This makes it challenging to compose with additional workflow steps, because the (contained) ci-fusesoc-action python environment into which fusesoc runs is different from the main workflow's one and wouldn't have the python package installed from previous steps, see https://github.com/proppy/misato/blob/f81bdeb1592000aaf70e7ed9af5071fe5cbc1136/.github/workflows/openlane.yml#L15 which currently failes with a ModuleNotFoundError: No module named \'amaranth\' error: https://github.com/proppy/misato/runs/4897990390?check_suite_focus=true

Since 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 to fusesoc itself if the corresponding tools 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!

@umarcor
Copy link

umarcor commented Jan 22, 2022

@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, community/community#9049 and community/community#9053) or Containers Actions (see community/community#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.

@umarcor
Copy link

umarcor commented Jan 22, 2022

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.

@proppy
Copy link
Author

proppy commented Jan 22, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants