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

Adds support for GitHub Actions #22

Conversation

marceltaeumel
Copy link
Member

@marceltaeumel marceltaeumel commented Sep 22, 2021

Here is an example on what it will look like:
https://github.com/marceltaeumel/squeak-app/actions/runs/1262385209

Details:

  • Drops support for TravisCI
  • Drops support for Squeak 5.0 and 5.1
  • Splits up prepare.sh into prepare_image.sh, test_image.sh, prepare_bundles.sh, and deploy_bundles.sh
  • Extracts prepare_image_post.sh from (original) prepare_image.sh to be used during bundle creation (such as for collecting the extra Etoys project files)
  • For testing, patches smalltalkCI to support GHA via gha-support.cs in test_image.st (can be removed later; see Add support for GitHub Actions hpi-swa/smalltalkCI#541)
  • Uses Windows platform to prepare both 32-bit and 64-bit images, and to test the images
  • Uses macOS platform to create (and sign/notarize) and deploy the bundles
  • All intermediate build artifacts will be uploaded during the action to remain inspectable for 90 days; this does not replace the deployment step to files.squeak.org

Next steps:

  • bundle.yml: Update push: filter to only work on "squeak-trunk"
  • prepare_bundles.sh: Provide secrets to prepare signing and ssh/rsync
  • prepare_aio.sh: Provide secrets to sign and notarize the all-in-one for macOS
  • prepare_mac.sh: Provide secrets to sign and notarize the all-in-one for macOS
  • deploy_bundles.sh: Provide secrets for rsync
  • Merge changes back to "squeak-5.1" and "squeak-5.2" branches, then updating build matrix, push filter, and DEPLOYMENT_BRANCH in each branch
  • Enable mail delivery to [email protected]

I will take a look at sign/notarize soon. Maybe one of you is more experienced in this field already and can thus help out. :-)

…s Squeak 5.0 and 5.1:

- Splits up prepare.sh into prepare_image.sh, test_image.sh, prepare_bundles.sh, and deploy_bundles.sh
- Extracts prepare_image_post.sh from (original) prepare_image.sh to be used during bundle creation (such as for collecting the extra Etoys project files)
- For testing, patches smalltalkCI to support GHA via gha-support.cs in test_image.st (can be removed later)
- Uses Windows platform to prepare both 32-bit and 64-bit images, and to test the images
- Uses macOS platform to create (and sign/notarize) and deploy the bundles
- All intermediate build artifacts will be uploaded during the action to remain inspectable for 90 days; this does not replace the deployment step to files.squeak.org
Copy link
Contributor

@LinqLover LinqLover left a comment

Choose a reason for hiding this comment

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

Not a review, even though GitHub will label it like one, just some curious questions. :-)

Can we also address this issue (duplicate squeak.sh file) or should this go into a separate PR?

As part of this change, could we also consider replacing the recurring "Hi all!" commit by a GitHub action that sends the failure mail to the list (something like this)? I think this would make the commit history look cleaner and it's also more transparent. :-)

Comment on lines 11 to 17
pull_request:
branches:
- squeak-trunk
# - squeak-5.3
# - squeak-5.2
paths-ignore:
- '**.md'
Copy link
Contributor

Choose a reason for hiding this comment

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

If we build on pushes to every branch, why do we need double builds for every PR then?

Copy link
Member Author

@marceltaeumel marceltaeumel Sep 23, 2021

Choose a reason for hiding this comment

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

Both push and pull_request should have the same filter. Please read my comments above. This is due to being able to try out the builds in my fork. The one who merges this PR still has to do changes to afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was rather questioning the existence of either push or pull_request in general. :-) I am usually going with just push to any branch. Or wouldn't this work for forks then?

Comment on lines 46 to 55
- name: Upload artifacts
uses: actions/upload-artifact@v2
with:
name: ${{ matrix.smalltalk }}
path: |
tmp/*.sources
tmp/*.image
tmp/*.changes
tmp/version.sh
!tmp/Test*
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of interest, where is the 90-day limit you mention indicated? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

In the repository settings. Only visible for admins.

env_vars Outdated
@@ -0,0 +1,14 @@
readonly HOME_DIR="$(pwd)"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you named this file with a .env suffix, editors such as VS Code could display them more conveniently, I think. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a source file. Same approach as in smalltalkCI. Maybe I should rename it? It's not even "export". Hmm...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for renaming it. For instance, I do not get shellcheck or syntax highlighting in VS Code if the file does not have a suffix.

Copy link
Member

Choose a reason for hiding this comment

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

I think just env is a very popular choice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, let's measure this! :-)

glob pattern hits on public GitHub repositories
env 2.8k
*.env 29.1k
?*env 12.5k
env_* 0.635k

So I guess anything but a file name ending in _env would be "popular". And VS Code should probably detect just env as environment variables, too. :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Just because it is hacktober, FYI: mikestead/vscode-dotenv#28 🙃

readonly TMP_DIR="${HOME_DIR}/tmp"

readonly FILES_BASE="http://files.squeak.org/base"
readonly RELEASE_URL="${FILES_BASE}/${SMALLTALK_VERSION/Etoys/Squeak}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there an Etoys?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is still support for building Etoys bundles. See http://files.squeak.org/etoys/ . It uses the same base files from files.squeak.org/base. So, you have to replace "Etoys" with "Squeak" when building for "Etoys-trunk" or "Etoys64-trunk".

@marceltaeumel
Copy link
Member Author

Can we also address this issue (duplicate squeak.sh file) or should this go into a separate PR?

Since this is not directly related to supporting GHA, this would be a separate PR.

As part of this change, could we also consider replacing the recurring "Hi all!" commit by a GitHub action that sends the failure mail to the list (something like this)? I think this would make the commit history look cleaner and it's also more transparent. :-)

Well, GitHub Actions has no support for e-mail notification yet. Using one of the existing mail-send actions, we would need a custom e-mail account for such things. We don't have one at the moment. I would rather not use a personal one for such things.

The recurring "Hi all!" commit is not needed anymore because only Travis had those e-mail notifications, which we used for scheduled runs.

@marceltaeumel marceltaeumel force-pushed the marceltaeumel/squeak-trunk-actions branch from 95752ca to 41d5fd6 Compare September 25, 2021 18:22
@marceltaeumel marceltaeumel merged commit 03763b1 into squeak-smalltalk:squeak-trunk Sep 25, 2021
@LinqLover
Copy link
Contributor

With a bit of delay, thanks for the explanations! :-)

As part of this change, could we also consider replacing the recurring "Hi all!" commit by a GitHub action that sends the failure mail to the list (something like this)? I think this would make the commit history look cleaner and it's also more transparent. :-)

Well, GitHub Actions has no support for e-mail notification yet. Using one of the existing mail-send actions, we would need a custom e-mail account for such things. We don't have one at the moment. I would rather not use a personal one for such things.

I already have a working dummy account for exactly this purpose (already using this for some other repos). If you are interested, I can DM you the credentials. :-)

Can we also address this issue (duplicate squeak.sh file) or should this go into a separate PR?

Since this is not directly related to supporting GHA, this would be a separate PR.

Just created #23, will do at any time later if no one else anticipates me. :-)

LinqLover added a commit to LinqLover/vscode-dotenv that referenced this pull request Oct 4, 2021
There is a large number of dotenv files out there that do not
actually use a dot in their name. Here is a small comparative study:

- `env`: 2.8k
- `*.env`: | 29.1k
- `?*env`: 12.5k
- `env_*`: 635

For more information on the study, see:
squeak-smalltalk/squeak-app#22 (comment)
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.

3 participants