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

Fixes artifact upload for platform cross building #165

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Sep 18, 2023

This PR is an upstream port of typelevel/sbt-typelevel#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 typelevel/sbt-typelevel@1801859 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).

Copy link
Collaborator

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

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.

@armanbilge
Copy link
Contributor

@mdedetrich
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@mdedetrich
Copy link
Contributor Author

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

mdedetrich and others added 5 commits October 23, 2023 20:16
(cherry picked from commit 1d3ab46c3830bcd4ed224776178c034b149edd26)
(cherry picked from commit 347b0640da74692626127bd91568c582b4e525c7)
(cherry picked from commit 94e06227498a7f95d1d938bde5ef980d5fe7fd14)
@mdedetrich
Copy link
Contributor Author

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 typelevel/sbt-typelevel@5b86e38 . 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?

@eed3si9n
Copy link
Member

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.

@mdedetrich mdedetrich changed the title Cross aware artifact upload Fixes artifact upload for platform cross building Oct 23, 2023
@armanbilge
Copy link
Contributor

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 👍

(cherry picked from commit 5b86e388b09753349138ce453933532459fe14c7)
@mdedetrich
Copy link
Contributor Author

that seems fine 👍

@armanbilge So I have just cherry picked typelevel/sbt-typelevel@5b86e38 without the mergify changes but as you can see in 7963523#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?

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.

4 participants