search

gap-actions / setup-gap

Other
1 stars 5 forks source link

Split this action: separate "installing GAP" from "building the package #21

Closed fingolfin closed 3 years ago

fingolfin commented 3 years ago

This allows for more flexible use of this action (resp. the >= 2 actions replacing it). After all, there were/are reasons why those are separate in the pkg-ci-scripts.git scripts.

E.g. it helps simplify is installing additional binary dependencies needed for compiling the package when ABI=32: If building GAP and building the package is separate, then I can insert the installation of the additional dependencies between those two steps; then I don't need to sudo dpkg --add-architecture i386 && sudo apt-get update.

It also is helpful when debugging CI failures: When looking at the logs, right now the compilation of GAP, io, profiling and other packages are lumped together with that of the package being tested, making it annoying to track down the error messages. Also, it's nice to be able to differentiate at one glance whether the failure is in m package, or already in GAP / one of the other packages being built.

Of course the v1 action should be left as-is so that existing CI setups don't break.

fingolfin commented 3 years ago

The new action could be called build-package or just build-pkg; and this action here could be renamed to setup-gap (I am not sure if that would break existing users or not; GitHub does add redirects for the git repo when you rename it, but I am not sure if that's sufficient for GitHub Actions, too. One could just try out what happens, and if it breaks stuff, quickly revert it.

ssiccha commented 3 years ago

I've blocked a few hours today in the early evening to work on this.

ssiccha commented 3 years ago

Fixed by 1716bfe.

ssiccha commented 3 years ago

Ah I'll just the leave as it is because

  1. we only use this action for packages (at least for now)
  2. more or less all other actions are also called ..-for-packages.
fingolfin commented 3 years ago

That the names end in "..-for-packages" actually seems weird to me. It doesn't seem to fit with how GitHub Actions are named. Let's look at the three actions defined here with this suffix (so "more or less all other actions" refers to just two actions ;-).

  1. run-test-for-packages... which packages? we only run tests for one packages... and actualy, we run tests, plural, so shouldn't it be run-tests-for-package? IMHO even nicer would be run-tests or run-package-tests or run-pkg-tests or so
  2. compile-documentation-for-packages: again, why "packages"? And why "compile", I think we never use that verb in this context elsewhere, do we? Usually it's "build" or "make"... so maybe "build-docs" or build-package-docs or build-pkg-docs (I'd just make it
  3. setup-gap-for-packages: here at least one could argue that "packages" (plural) makes sense as the GAP being set up is also used to build io and profiling. But that seems rather far fetched... Hence my suggestion to call it setup-gap.

Of course to a degree this is bikeshed painting! But names are important, too, so some discussion on this is IMHO seems appropriate. That said, I won't loose sleep if we keep the existing names, and I do not mean to bludgeon you into this. But let's at least not use the existing names blindly as justification for naming further actions.

ssiccha commented 3 years ago

I've renamed the actions mentioned above. :+1:

fingolfin commented 3 years ago

Now we "just" should submit PRs to all affected packages to use the new names ... ah, if only I had waited with my CI PRs till after we renamed and split everything 😂 but then again I only discovered the usefulness of some splits while trying to use the actions, so... At the very least updating the example package accordingly would be nice