Skip to content

Cross-build to sbt 2#219

Open
anatoliykmetyuk wants to merge 5 commits intosbt:mainfrom
anatoliykmetyuk:sbt2-port
Open

Cross-build to sbt 2#219
anatoliykmetyuk wants to merge 5 commits intosbt:mainfrom
anatoliykmetyuk:sbt2-port

Conversation

@anatoliykmetyuk
Copy link
Copy Markdown
Member

No description provided.


ThisBuild / githubWorkflowJavaVersions := Seq(
JavaSpec.temurin("8"),
// Java 17 first: publish job uses the head of this list when downloading staged artifacts; sbt 2 (Scala 3 axis) needs 17+.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW I'm proposing to make Java 17 the minimum in #205 but haven't dared pull the trigger yet.

Copy link
Copy Markdown
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Nice! Some comments.

build.sbt Outdated
ThisBuild / scmInfo := Project.scmInfo
ThisBuild / description := Project.description

def sbtVersionForPlugin(scalaBinary: String): String =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe name this clearer to reflect that this is the version we run the scripted tests with?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This one is not only for the scripted tests but also for the plugin itself - it is used twice in this file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, right, I didn't notice. In that case are we sure we want these to be the same? I'd say we'd want to build with the "latest" sbt but test against the "minimum supported".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok, fixed this. Testing against minimal sbt 1.6.0 as per README, and 2.0.0-RC11 for sbt 2. I don't think it's reasonable to specify a minimal supported version for sbt 2 to be an RC version; we should wait until 2.0.0 and set the minimal to it.

tar xf targets.tar
rm targets.tar

- name: Download target directories (3.8.2)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

using the cache is an attack vector and this job is also used for releases - does it really make a big difference? Does this come in via githubWorkflowGenerate? Could we disable it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This behaviour was already in the build before this PR: see line https://github.com/sbt/sbt-sbom/pull/219/changes#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR158 which does the same for Scala 2.12.

We could disable this behavior with ThisBuild / githubWorkflowArtifactUpload := false which would make the publish job run slower but will remove caching. WDYT?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha. I think that would be good - we're not so active that it matters a lot for us anyway

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed, now the caching is disabled.

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