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] cleanup #54

Merged
merged 6 commits into from Jun 3, 2021
Merged

[ci] cleanup #54

merged 6 commits into from Jun 3, 2021

Conversation

umarcor
Copy link
Collaborator

@umarcor umarcor commented Jun 3, 2021

This is a cleanup of the bash scripts used in the 'Processor' CI workflow.

  • .ci/install.sh is removed because it can be a two-liner written in the workflow.
  • .ci/hw_check.sh is removed because it's just a wrapper around sim/ghdl/ghdl_sim.sh.
  • .ci/sw_check.sh is moved to sw/check.sh.
  • The content of .ci/README.md is split to sw/check.sh and sim/ghdl/ghdl_sim.sh.
  • Shell scripts are made executable, so that chmod is not required.
  • In sim/ghdl/ghdl_sim.sh, GHDL's import and make commands are used, instead of analyse and elaborate. That allows not to specify the sources one by one. GHDL resolves the compilation order internally.

NOTE: @stnolting, on windows, you can use git update-index --chmod=+x path/to/script for marking an script as executable in git. Then commit and push. Anyone who clones the repo will have permissions properly set.

@stnolting
Copy link
Owner

Nice clean-up! 👍 I really like this, but there is just one remark:

.ci/sw_check.sh is moved to sw/check.sh.

I think this should stay in .ci because it is only relevant for the workflow-based "verification". Or maybe this could also be directly in the according application folder sw/example/processor_check. I think it might be confusing having it in the sw folder. What do you think?

In sim/ghdl/ghdl_sim.sh, GHDL's import and make commands are used, instead of analyse and elaborate. That allows not to specify the sources one by one. GHDL resolves the compilation order internally.

Awesome! I did not know that. I always had problems with the compile order when the packages were not compiled first.

NOTE: @stnolting, on windows, you can use git update-index --chmod=+x path/to/script for marking an script as executable in git. Then commit and push. Anyone who clones the repo will have permissions properly set.

Thank for the hint! 😉

@stnolting
Copy link
Owner

By the way, what do you think about having the pre-build GCC toolchain as submodule inside this repository? Maybe in sw/gcc?
Tha scripts (makefiles) could use that as default so new-user could do a simple recursive git clone and they would get everything as a bundle 🤔

@umarcor
Copy link
Collaborator Author

umarcor commented Jun 3, 2021

.ci/sw_check.sh is moved to sw/check.sh.

I think this should stay in .ci because it is only relevant for the workflow-based "verification". Or maybe this could also be directly in the according application folder sw/example/processor_check. I think it might be confusing having it in the sw folder. What do you think?

Honestly, I was not sure about it...

On the one hand, I don't think that subdir .ci makes "whole sense" because now it contains a single file. That is, these scripts are either for testing in general, or specific for GitHub Actions; they were not really written for any CI service. That's why I feel they belong in either .github, .github/sh, or close to the sources they are testing. Since the hardware test is already located un subdir sim, I thought it'd be coherent to have this check in the sw subdir.

On the other hand, even though a single check is done at the moment, I guess you might want to add further checks in the future.

Anyway, I now moved it to sw/example/processor_check, as you suggested. I, the future, I think that we might merge it into sw/example/processor_check/Makefile.

Awesome! I did not know that. I always had problems with the compile order when the packages were not compiled first.

IIRC there are specific combinations which are poorly defined in the language. Hence, sometimes, GHDL might still have issues for guessing the whole compilation order. However, I found it to be quite reliable, since the default synthesis command relies on that when all sources are provided at once. Moreover:

Thank for the hint! 😉

When I started contributing to GHDL, Tristan had to fix all the executables for me 😆, until I learnt that trick 😄.

By the way, what do you think about having the pre-build GCC toolchain as submodule inside this repository? Maybe in sw/gcc?
Tha scripts (makefiles) could use that as default so new-user could do a simple recursive git clone and they would get everything as a bundle 🤔

That's something I wanted to bring in a different issue 🤣. So, why are you using your pre-built GCC toolchain? Do you need to customise it so that it understands NEORV32 as the target architecture? Or is it because of convenience? That is, could you use an "upstream" GCC? May NEORV32 be upstreamed?

This is a tool which your project depends on. It should not be a submodule inside the repository. Furthermore, most users should not install it through a custom script in this repo. Similarly to GHDL, Yosys, nextpnr, VUnit, etc. those should be installed independently. We want this project (and potentially any open source project) to be usable very regardless of the preferred environment of the user. Some will install system packages, others will use containers, MSYS2, Conda, Homebrew... They don't want to install those tools each time, but (very) infrequently. In that context, you are currently providing a single build for a very specific subset of environments.

Please, don't take me wrong. This is absolutely normal. Most open source EDA tooling projects are not properly packaged yet. Some (a few) are available on some (a few) popular distributions. There are efforts such as HDL's bazel and conda repos, or YosysHQ's oss-cad-toolchain. Yet, each of them is an ecosystem for the domain the maintainers/supporters are interested in.

Should you need to distribute a custom GCC because you need it customised, I would recommend that we add the generation of a container image and an MSYS2 package to you other repo. Then, consume those as any user will do. In fact, this is one of the main purposes of CI: to reproduce the experience you expect users to have on different platforms.

@stnolting
Copy link
Owner

On the other hand, even though a single check is done at the moment, I guess you might want to add further checks in the future.

Good point. But let's just leave it in sw/example/processor_check for now.

IIRC there are specific combinations which are poorly defined in the language. Hence, sometimes, GHDL might still have issues for guessing the whole compilation order. However, I found it to be quite reliable, since the default synthesis command relies on that when all sources are provided at once. Moreover:

👍

That's something I wanted to bring in a different issue 🤣. So, why are you using your pre-built GCC toolchain? Do you need to customise it so that it understands NEORV32 as the target architecture? Or is it because of convenience? That is, could you use an "upstream" GCC? May NEORV32 be upstreamed?

Then let's discuss that in a new issue. 😄

This is a tool which your project depends on. It should not be a submodule inside the repository.

Yeah right. You convinced me. 😉

@umarcor
Copy link
Collaborator Author

umarcor commented Jun 3, 2021

Good point. But let's just leave it in sw/example/processor_check for now.

Agree! Then, this is ready to merge 😉

Then let's discuss that in a new issue. 😄

👌🏼 #55

@stnolting stnolting merged commit eb93247 into stnolting:master Jun 3, 2021
@umarcor umarcor deleted the ci/cleanup branch June 3, 2021 16:04
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

Successfully merging this pull request may close these issues.

None yet

2 participants