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

Update and test the getting-started script #5446

Merged
merged 8 commits into from
Sep 5, 2024
Merged

Conversation

rzadp
Copy link
Contributor

@rzadp rzadp commented Aug 23, 2024

Here are some changes to the getting-started.sh scripts we have advertised on top of the readme.

Changes to the script

  1. Change echo to a more portable printf.

On my machine, the script printed a literal \n string if run with bash.
If I changed it to echo -e, then it printed a literal -e if run with sh.

Changed it to printf which is more portable.


  1. Template selection

The script proceeded to clone and build the minimal template, which is not always what we want.
Added a selection prompt where the user can select one of the 3 templates, and choose if it should be built&run or not.
The user can also select no template at all - that way, we have a starter of a dependencies-installation script.


  1. Added some missing dependencies for some of the systems.

A workflow testing the script

I propose a workflow, that will test the script using the expect tool.
For each OS mentioned in the script (macOS, Ubuntu, Debian, Arch, Fedora, OpenSUSE) we go through the script twice, and after that build and run the template binary.

I'm using docker containers, so we start from scratch and make sure the scripts installs all required dependencies - with the exception of macOS, which I can't run from scratch in a container.

The jobs use a selected combination of OSes, shell interpreters (bash or sh), and templates.
There is too much combinations to run them all, but I have run it once in staging to make sure all pass.

I'm adding a cron schedule because it can break without any code changes in this repository (e.g. new latest release of a container).

@rzadp rzadp requested review from a team as code owners August 23, 2024 09:53
@rzadp rzadp added the R0-silent Changes should not be mentioned in any release notes label Aug 23, 2024
@kianenigma
Copy link
Contributor

Template selection

In this section, when they cancel, can you also make sure to show them "if you want more templates, please see here"?

A workflow testing the script

I am not an expert to judge the complexity of the testing scripts. I can share the overall guidance that we should also not over-engineer it, both to minimize the CI costs and the maintenance of the scripts.

That is not to say the current steps are over-engineered per-se, this is more of an overall thought.

What I would suggest is only to remove the last two steps: The templates are already checked to compile, run and produce blocks elsewhere. We don't need it here again.

As noted elsewhere, if we can use the same getting started script to setup our CI environment, it will be a double win, and more incentive to battle-test it.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, but nothing major.

@rzadp
Copy link
Contributor Author

rzadp commented Aug 23, 2024

What I would suggest is only to remove the last two steps: The templates are already checked to compile, run and produce blocks elsewhere. We don't need it here again.

Hmm, I'm not sure we're on the same page here.
Yes, the templates are checked to compile, run and produce blocks elsewhere, but only on Ubuntu/Debian and macOS.

What this introduces, is making sure that those exact lists of dependencies and Rust setup is enough to build a template on a clean arch/fedora/opensuse.

But yeah, maybe this is not a place for it. 🤔

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

.github/workflows/check-getting-started.yml Outdated Show resolved Hide resolved
.github/workflows/check-getting-started.yml Outdated Show resolved Hide resolved
.github/workflows/check-getting-started.yml Outdated Show resolved Hide resolved
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
@rzadp rzadp added this pull request to the merge queue Sep 5, 2024
Merged via the queue into master with commit 5e0ec3e Sep 5, 2024
199 of 208 checks passed
@rzadp rzadp deleted the rzadp/check-getting-started branch September 5, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants