sbt-github-actions icon indicating copy to clipboard operation
sbt-github-actions copied to clipboard

Fixes artifact upload for platform cross building

Open mdedetrich opened this issue 2 years ago • 8 comments

This PR is an upstream port of https://github.com/typelevel/sbt-typelevel/pull/98, every commit has been cherry picked both for authorship acknowledgement and also to make it easy to correlate the commits. In the process of cherry picking I have also done modifications so that the project compiles and the tests run (i.e. for each cherry picked commit I ran sbt githubWorkflowGenerate for both the root build and every sbt scripted test). Exceptions are noted below

  • I have added the Handle case where githubWorkflowBuildMatrixAdditions is empty commit myself as this appears to be an accidental regression. In sbt-typelevel there doesn't seem to be a case where githubWorkflowBuildMatrixAdditions is empty but this is typically the case for standard trivial builds and the github workflow generation in this case appeared to be incorrect (i.e. trailing - and including rootJVM by itself when not necessary). Please check if this is intended, particularly the removal of the rootJVM part (I may be missing something here)
  • I did not cherry pick the https://github.com/typelevel/sbt-typelevel/pull/98/commits/1801859216cca487d7fedb125ca08ecad9a8b55c commit as this appeared to be specific for typelevel-sbt and how they structure their projects. We should probably add a new commit that either modifies an existing test or makes a new one so we set githubWorkflowArtifactDownloadExtraKeys to some value to test that it works @djspiewak @armanbilge wdyt? Feel free to just add the commit onto this PR if you want to do it yourself (maintainers can add commits onto this PR).

mdedetrich avatar Sep 18 '23 09:09 mdedetrich

I think there's been additional fixes since the original PR. https://github.com/typelevel/sbt-typelevel/blob/5dec904d36c0b2cceb3f18b9ed7323ad849614a4/github-actions/src/main/scala/org/typelevel/sbt/gha/GenerativePlugin.scala#L725

armanbilge avatar Sep 18 '23 15:09 armanbilge

I think you made the right choice on cherry picking. This looks good to me apart from the fact that I'd ideally like to see a test for it, but I'm okay either way.

Agreed about test, was just leaving it open if someone else wanted to suggest/add one on their own but if not I will add one later.

I think there's been additional fixes since the original PR. https://github.com/typelevel/sbt-typelevel/blob/5dec904d36c0b2cceb3f18b9ed7323ad849614a4/github-actions/src/main/scala/org/typelevel/sbt/gha/GenerativePlugin.scala#L725

Thanks, I just picked a PR from git blame but evidently I missed some stuff. Will look into it later

mdedetrich avatar Sep 18 '23 15:09 mdedetrich

I just came back from a wedding, ill try and look into this in the next few days.

mdedetrich avatar Sep 29 '23 11:09 mdedetrich

Now that the other stuff is out of the way I am going to look into this

mdedetrich avatar Oct 23 '23 17:10 mdedetrich

I think there's been additional fixes since the original PR. https://github.com/typelevel/sbt-typelevel/blob/5dec904d36c0b2cceb3f18b9ed7323ad849614a4/github-actions/src/main/scala/org/typelevel/sbt/gha/GenerativePlugin.scala#L725

@armanbilge So I had a look and one of the changes which I think you are referring to is this commit https://github.com/typelevel/sbt-typelevel/commit/5b86e388b09753349138ce453933532459fe14c7 . The problem here is that it seems to also bring in Mergify changes (which doesn't exist in sbt-github-actions yet), if I cherry pick that commit I assume its just safe to ignore all of the Mergify changes or would I be missing something?

mdedetrich avatar Oct 23 '23 18:10 mdedetrich

As a nit on the title, I suggest calling this "Fixes artifact upload for platform cross building" or something since "cross" alone often suggests Scala cross building.

eed3si9n avatar Oct 23 '23 19:10 eed3si9n

if I cherry pick that commit I assume its just safe to ignore all of the Mergify changes or would I be missing something?

that seems fine 👍

armanbilge avatar Oct 23 '23 20:10 armanbilge

that seems fine 👍

@armanbilge So I have just cherry picked https://github.com/typelevel/sbt-typelevel/commit/5b86e388b09753349138ce453933532459fe14c7 without the mergify changes but as you can see in https://github.com/sbt/sbt-github-actions/pull/165/commits/796352371f8ffd6545c87e9b31f67078a284354f#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR137-R186 the resulting ci.yaml is highly suspect, there are duplicate entries for downloading/inflating target directories

Maybe I am missing an earlier commit that fixes some sought of bug or?

mdedetrich avatar Oct 24 '23 08:10 mdedetrich