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

Feat/support projectmatrix and support better crossbuilding #440

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

Conversation

ThijsBroersen
Copy link

@ThijsBroersen ThijsBroersen commented Nov 3, 2024

This PR contains quite some changes but tries to minimize the impact for the user.

After working with multiple zio-repos I noticed the build and ci setups have quite some duplication (copy paste) but also quite some differences (diverging over time). With this PR I hope the projects can scrap a lot of their own boilerplate and reuse as much as is convenient from the plugins from zio-sbt.

There are multiple projects which are building JS, JVM and Native separately in CI, this PR now does that by default by including the platforms in the ci-matrix.
The

I added two new plugins: ZioSbtProjectPlugin and ZioSbtShared

ZioSbtShared contains banner and tasks utils. Easy helpers to show info to the user.

ZioSbtProjectPlugin sets a default javaPlatform to target when building artifacts by setting the -release flag.
The plugin also adds a banner to show what java and scala versions and platforms are being used in the project.

e.g.

Scala 2.12.x 2.13.15 3.3.4
JS
JVM
Native

I have added multiple compile and test tasks:

Platform Scala 2.12 Scala 2.13 Scala 3 All
JVM compileJVM2_12 compileJVM2_13 compileJVM3 compileJVM
Scala.js compileJS2_12 compileJS2_13 compileJS3 compileJS
Scala Native compileNative2_12 compileNative2_13 compileNative3 compileNative
All compile2_12 compile2_13 compile3 compile
Platform Scala 2.12 Scala 2.13 Scala 3 All
JVM testJVM2_12 testJVM2_13 testJVM3 testJVM
Scala.js testJS2_12 testJS2_13 testJS3 testJS
Scala Native testNative2_12 testNative2_13 testNative3 testNative
All test2_12 test2_13 test3 test

When using sbt-projectmatrix the idea is to have all in the same build scope. You will see the welcome message showing tasks for all versions and platforms available. If a compile or test task is tried for a something which is not in the current context then it will be an empty task.

ZioSbtCiPlugin I extended here and there. Based on the platforms and scala versions it will now create a matrix for the ci jobs as default behaviour.
It is now also possible to set a java distribution via ciDefaultJavaDistribution.

Check the README.md for other changes.

note: this PR also contains migrations to zio-json, as I continued on my previous PR #438

@ThijsBroersen ThijsBroersen force-pushed the feat/support-projectmatrix-and-support-better-crossbuilding branch from 36f8dcf to 6f4cdd4 Compare November 3, 2024 14:12
@ThijsBroersen
Copy link
Author

ThijsBroersen commented Nov 3, 2024

What I would like to do is to create some draft PRs for other ZIO projects to show the results. Can someone publish some snapshots from this PR perhaps?
I published my own fork and made some draft, example PRs:
zio/zio-logging#908
zio/zio-json#1190
zio/zio-config#1466
zio/zio-kafka#1397

.value,
doc / skip := BuildAssertions.requireNative(true).value,
Compile / doc / sources := BuildAssertions.requireNative(Seq.empty).value
// Test / test := { val _ = (Test / compile).value; () } // ??
Copy link
Author

Choose a reason for hiding this comment

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

why was this previously set? What is the benefit?

@ThijsBroersen ThijsBroersen force-pushed the feat/support-projectmatrix-and-support-better-crossbuilding branch from 92e42b4 to 6278bc7 Compare November 16, 2024 09:06
@ThijsBroersen ThijsBroersen force-pushed the feat/support-projectmatrix-and-support-better-crossbuilding branch 3 times, most recently from 492eead to a4c166f Compare November 16, 2024 13:30
_.flatten
) ++
Seq(Lint.value) ++
(if (ciCheckMima.value) Seq(CheckMima.value) else Seq.empty[Step.SingleStep])
Copy link
Author

Choose a reason for hiding this comment

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

Perhaps it is better to have dedicated mima jobs? Per Scala version/platform?

Copy link
Author

Choose a reason for hiding this comment

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

checkMima is now part of the lint job. Can be disabled with checkMima / skip := true

@ThijsBroersen ThijsBroersen force-pushed the feat/support-projectmatrix-and-support-better-crossbuilding branch from f9cbda1 to a2bad89 Compare November 17, 2024 21:12
@khajavi
Copy link
Member

khajavi commented Nov 27, 2024

Hello @ThijsBroersen

Thank you for your interest in contributing to zio-sbt! We appreciate your efforts, but I would like to address a few concerns regarding your pull request:

  1. It would be helpful if you could submit a few smaller PRs instead of one large PR. Each smaller PR should focus on resolving a specific issue or adding a particular feature. This approach makes the review process easier.

  2. Breaking the changes into smaller PRs will allow us to identify which changes may be breaking and which ones are not.

  3. Currently, most projects within the zio ecosystem depend on this plugin. We want to ensure that all of them can migrate to the newer version smoothly. Do you have a plan to address this concern?

Thank you!

@ThijsBroersen
Copy link
Author

ThijsBroersen commented Nov 27, 2024

Hi @khajavi

Thanks for your comments.

1,2. Please specify what you want to break down. I know this PR includes the zio-json changes but I was waiting too long on responses on that PR and wanted to continue. Main is still broken and my zio-json branch is what I continued on. The zio-json changes duplicated the workflow models and I did not get any further replies on what direction to go.
I think it would be helpful to have a call and exchange some thoughts.. V4 of zio-sbt also made some breaking changes so perhaps changing some of the ScalaWorkflow api isn't that big of a problem. Having it close to the gha syntax makes it easier to work with.

  1. As I stated, I worked with multiple zio-projects and all are using zio-sbt quite differently. Most are far from adopting it in full and need migrations of the existing tags anyway. I made multiple demo, draft PR's (base on artifacts published from my own fork) to show the changes and I am happy to turn them into real PR's and perhaps try to even migrate more projects.
    I see a lot of duplicated boilerplate between the zio projects and with this PR I think we could remove a lot of that and push for more up-to-date projects.

Perhaps we should align in a call on the directions of this project. Ideas are always bouncing in my head but the PR interactions are just so slow...

@khajavi
Copy link
Member

khajavi commented Nov 27, 2024

1,2. Please specify what you want to break down. I know this PR includes the zio-json changes but I was waiting too long on responses on that PR and wanted to continue. Main is still broken and my zio-json branch is what I continued on. The zio-json changes duplicated the workflow models and I did not get any further replies on what direction to go.

Why did you close that PR? Would you like to start by migrating to zio-json PR?

3. As I stated, I worked with multiple zio-projects and all are using zio-sbt quite differently. Most are far from adopting it in full and need migrations of the existing tags anyway. I made multiple demo, draft PR's (base on artifacts published from my own fork) to show the changes and I am happy to turn them into real PR's and perhaps try to even migrate more projects.
I see a lot of duplicated boilerplate between the zio projects and with this PR I think we could remove a lot of that and push for more up-to-date projects.

There is no issue with broken changes, which is why we are conducting alpha releases. However, we must ensure that all projects can eventually migrate to the new version. The way you utilized those snapshots can indicate whether we are on the right path or not.

Perhaps we should align in a call on the directions of this project. Ideas are always bouncing in my head but the PR interactions are just so slow...

No problem! We can schedule a call to discuss the program and its future direction. ZIO SBt requires more contributors, and I'm glad you are interested in it.

@ThijsBroersen
Copy link
Author

ThijsBroersen commented Nov 27, 2024

Why did you close that PR? Would you like to start by migrating to zio-json PR?

Because I was making quite some more changes and didn't want the pain to keep working and rebasing between two branches. And to prevent it from getting out of sync with changes from this PR and being merged.

At the current state of this PR I think I can extract the zio-json changes into a separate PR, but I will have to squash and rebase this PR onto it.
It would help if we can first make a choice between the ScalaWorkflow models.

extend welcome message based on build settings
add checkMima task and add to lint command
support custom jdk distributions in ci job
@ThijsBroersen ThijsBroersen force-pushed the feat/support-projectmatrix-and-support-better-crossbuilding branch from a2bad89 to cc8e5fc Compare November 27, 2024 20:11
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.

2 participants