hpi-swa / smalltalkCI

Framework for testing Smalltalk projects with GitHub Actions, GitLab CI, Travis CI, AppVeyor, and others.
MIT License
94 stars 68 forks source link

Iceberg registering in github actions #582

Closed hogoww closed 1 year ago

hogoww commented 1 year ago

Hello,

This PR "fixes" #581 by redirecting users to the setup-SmalltalkCI link on which I will do another PR to add the requirements to make Iceberg work in github actions. Basically, we need to fetch all of the commits because Iceberg always creates a full history as far as I can tell. This is achieve with the option fetch-depth: 0 for actions/checkout@v2

- uses: actions/checkout@v2
        with:
          fetch-depth: 0

(Will do that PR later today)

I also put Iceberg in a preload rather than postload, to be able to use it during the loading. (In my case, this was necessary for resource usage). If you think there should be another explanation on how to Iceberg in the CI, let me know which file I should add it to.

Thanks again to @theseion for the support & @fniephaus for the solution !

hogoww commented 1 year ago

Thanks a lot for the review ! (And nice catch !) Only remaining issue is the non-GHA builds)

fniephaus commented 1 year ago

Isn't there an entry missing for preLoad in: https://github.com/hpi-swa/smalltalkCI/blob/b58ba5b309013ccf1d8623dac82d0e4eb69d2568/repository/SmalltalkCI-Core.package/SmalltalkCI.class/methodProperties.json#L115-L138

The 32bit builds are currently failing because GHA runners have been bumped to Ubuntu 22.04. I'll take a look at this in a few days.

fniephaus commented 1 year ago

Please address https://github.com/hpi-swa/smalltalkCI/pull/582#issuecomment-1328007708. 32-bit builds should be working again.

hogoww commented 1 year ago

@fniephaus Sorry about all those small mistakes. Thanks for catching them !

hogoww commented 1 year ago

I finally found the time to look into it. I'm not sure whether that's appropriate to not pollute over CIs. Please let me know.

hogoww commented 1 year ago

CI looks stuck here, although the action seem completed in the action tab

hogoww commented 1 year ago

Discard last commit comment, this is a separate issue. This looks ready from a code standpoint, awaiting review :)

hogoww commented 1 year ago

@theseion Thanks for all the great suggestions ! Everything is integrated. I did one commit rather than commit all your suggestion to avoid multiple CI trigger and mostly useless commits.

hogoww commented 1 year ago

Done, I hope I haven't forgotten anything. I'll have to update the associated PR in the setup.

fniephaus commented 1 year ago

is this good to go now, @hogoww @theseion ?

hogoww commented 1 year ago

Good for me

Rinzwind commented 1 year ago

There seems to be a bug in the following:

https://github.com/hpi-swa/smalltalkCI/blob/f58749eb342884e7233046aacffdeb270240ca46/repository/SmalltalkCI-Pharo-Core.package/SmalltalkCIPharo9.class/instance/preLoad.st#L2-L4

The message #registerInIceberg is only understood by instances of SCIPharoMetacelloLoadSpec, it is not understood by instances of other classes in the SCIAbstractLoadSpec hierarchy (such as SCIPharoCustomScript).

fniephaus commented 1 year ago

I have reverted the merge, assuming this has caused failures on @Rinzwind's end. Please take a look, @hogoww.

hogoww commented 1 year ago

Of course, thank you @fniephaus !

A quick fix could be to simply add a #canUnderstand:. Would that be okay? I don't know much about the other supported systems

fniephaus commented 1 year ago

I'd like to avoid #canUnderstand:. Why not add a method returning false to SCIAbstractLoadSpec, which I believe is the super class of all load specs?

hogoww commented 1 year ago

I'd like to avoid #canUnderstand:. Why not add a method returning false to SCIAbstractLoadSpec, which I believe is the super class of all load specs?

I wanted to avoid non Pharo components. But if you're okay with that, that'd indeed be the easiest solution.

fniephaus commented 1 year ago

Yes, it's an easy and clean solution. 🙂

hogoww commented 1 year ago

Sure then !