NNPDF / pineappl

PineAPPL is not an extension of APPLgrid
https://nnpdf.github.io/pineappl/
GNU General Public License v3.0
12 stars 3 forks source link

Fix problem in release workflow #230

Closed cschwan closed 1 year ago

cschwan commented 1 year ago

The newly-created release workflow creates a new release on github and also creates binary libraries that can be downloaded.

Unfortunately there's a bug in there as the version used to create the release isn't properly read out:

https://github.com/NNPDF/pineappl/blob/d70b7d35d695f104083cc17f413605687a6a7782/.github/workflows/release.yml#L17-L19

This results in the shell variable $version being empty.

alecandido commented 1 year ago

In principle the string is interpreted by Bash, so the syntax should be correct. To test it, try repeating the line without redirection, at least we'll see it in the logs.

However, the syntax suggested by GitHub for setting the environment variable is slightly different:

           echo "version=${GITHUB_REF_NAME#v}" >> "$GITHUB_ENV"

https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-environment-variable

(i.e. wrapped by double-quotes, rather than using the dollar + braces - it seems hard to be relevant, but with shells I'm always doubting)

cschwan commented 1 year ago

I think the problem is rather:

The step that creates or updates the environment variable does not have access to the new value, but all subsequent steps in a job will have access.

:facepalm:

But this was already a workaround, because I couldn't find a way to globally set the environment variable. Do you have an idea, @AleCandido?

alecandido commented 1 year ago

I see.

As I said, I'm always doubting of shells, but even more of GitHub workflows, since the steps are not even pure shell steps (in principle they are, somehow, since the idea is that the extra features are preprocessed first, or just a consequence of the environment, but it is always tricky...).

alecandido commented 1 year ago

But this was already a workaround, because I couldn't find a way to globally set the environment variable. Do you have an idea, @AleCandido?

Do you mean something like this? https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#env

EDIT: the problem with it is that is not inside the shell, so you can not use Bash variables expansion

alecandido commented 1 year ago

I see two options to solve your problem:

  1. in that step use directly the original variable https://github.com/NNPDF/pineappl/blob/d70b7d35d695f104083cc17f413605687a6a7782/.github/workflows/release.yml#L20
               gh release create -d ${GITHUB_REF_NAME}
  2. you double the variable declaration:
               export version=${GITHUB_REF_NAME#v}
               echo "version=${GITHUB_REF_NAME#v}" >> "$GITHUB_ENV"

You can pick the one you consider less ugly.

EDIT: case 2. maybe would be cleaner with:

           export version=${GITHUB_REF_NAME#v}
           echo "version=${version}" >> "$GITHUB_ENV"
cschwan commented 1 year ago

Yes, that should work. We don't even need export, as it's useless with github jobs.

cschwan commented 1 year ago

It finally worked! It really was over 9000... Thanks for your help, @AleCandido!

alecandido commented 1 year ago

It finally worked! It really was over 9000... Thanks for your help, @AleCandido!

You're welcome :D

The worse thing of GitHub workflows is debugging them, also because the only official way to test them is committ&push-ing over and over.

Unofficially, there is this project: https://github.com/nektos/act. It is very popular, but I only tried it once, and it didn't work. Maybe @scarlehoff knows something more...

scarlehoff commented 1 year ago

No, as you can see here my workflow debugging routine is also the brute force one

(although there it was a bit cleaner because I could encapsulate the testing in a separate PR, with releases is a bit trickier :P)

cschwan commented 1 year ago

The good thing is that you can now install the C API without needing anything related to Rust, by simply typing anywhere in your terminal:

curl --proto '=https' --tlsv1.2 -sSf https://raw.githubusercontent.com/NNPDF/pineappl/master/install-capi.sh | sh -s 0.6.0-alpha.15
alecandido commented 1 year ago

Actually, I would even consider to deploy the script on GitHub pages (just as it is), to have a "more official" URL, with less details.

However, this is definitely not a priority.

cschwan commented 1 year ago

Good point, and it already is: https://nnpdf.github.io/pineappl/install-capi.sh! That's a much shorter URL!