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

Replace setup-scala with setup-java #2294

Merged
merged 1 commit into from
Jan 26, 2022
Merged

Replace setup-scala with setup-java #2294

merged 1 commit into from
Jan 26, 2022

Conversation

Philippus
Copy link
Member

No description provided.

@julienrf
Copy link
Contributor

Thank you @Philippus. Is it exactly equivalent? Are there any differences we should be aware of? Could you please apply the change here first? https://github.com/scalacenter/library-example

@SethTisue
Copy link
Member

some background: olafurpg/setup-scala#49

@Philippus
Copy link
Member Author

Philippus commented Jan 20, 2022

Created this PR: scalacenter/library-example#75

@sideeffffect
Copy link
Contributor

@julienrf
Copy link
Contributor

julienrf commented Jan 24, 2022

Looking at the diff, setup-java does not seem to be a win. I believe we should at least use coursier/setup-action or even japgolly/setup-everything-scala (which does use coursier/setup-action and does even more). On the one hand, I like the fact that japgolly/setup-everything-scala sets up caching out of the box, on the other hand, the fact that it also sets up NodeJS may slow down builds that don’t use that feature? I am not sure.

@Philippus
Copy link
Member Author

The main immediate win is Temurin, the follow-up to AdoptOpenJDK. Setup-scala uses Jabba which is lagging behind a lot (also doesn't offer JDK 17 at the moment). Setup-java is also suggested in the sbt docs: https://www.scala-sbt.org/1.x/docs/GitHub-Actions-with-sbt.html.

@sideeffffect
Copy link
Contributor

Yes, Jabba is bad 😞
https://github.com/japgolly/setup-everything-scala uses https://github.com/coursier/setup-action, which in turn uses https://github.com/coursier/coursier to install JVMs, and it uses Jabba's index. There's a ticket to use a better alternative to Jabba coursier/coursier#2057 but I don't know if @alexarchambault would agree to that.

Also, for general Scala usage, setting up JS stuff by default is not ideal. But I don't know if @japgolly would agree to having JS (and Native in the future) as opt-in.

@SethTisue
Copy link
Member

SethTisue commented Jan 24, 2022

I strongly believe that setup-coursier isn't a viable option until coursier/coursier#1957 is addressed. My understanding is that the Center considers it important and intends to address it.

And because setup-everything-scala uses setup-coursier as its starting point, it's affected by the same issue, and thus not currently a viable option either.

Regardless, I think setup-everything-scala is a great project, but I think it's too early to make it our flagship recommendation, especially since it's a one-person project. But I see a possible future in which the Center chooses to adopt it (with David's consent) and it becomes the standard. I don't think that day is here yet.

@japgolly
Copy link

japgolly commented Jan 26, 2022

Also, for general Scala usage, setting up JS stuff by default is not ideal. But I don't know if @japgolly would agree to having JS (and Native in the future) as opt-in.

Hello! Yeah if we decide that's the way to go then no problem at all, but before we get there I'd like to have a quick evaluation to understand what about it isn't ideal. Maybe whatever problems presented can be solved in another way so that we can have our cake and eat it too 😀 I'm going to assume the concern is about installation time in which case, you might want to have a look at the times in practice because the Scala.JS stuff doesn't seem to be significant. For example, the browsers are always preinstalled in the GH environments so unless a user specifies a specific version, there's no additional browser installation. To be clear, I'm not against your suggestion, I'd just like to explore it in a bit more detail first.

Regardless, I think setup-everything-scala is a great project, but I think it's too early to make it our flagship recommendation, especially since it's a one-person project. But I see a possible future in which the Center chooses to adopt it (with David's consent) and it becomes the standard. I don't think that day is here yet.

Thank you! And np, very fair concerns. I'm happy to accept additional contributors, and you have my consent to do whatever you think is best. Cheers

@SethTisue
Copy link
Member

@julienrf want to make the ruling on whether this PR (and the PR in scala-library-example) should proceed or not? I would lean towards merging them, but of course I'll be fine with whatever you decide.

Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

I agree that in the short term this change looks good.

I believe that, ultimately, we should have a single action that replaces:

    - name: Setup JDK
      uses: actions/setup-java@v2
      with:
        distribution: temurin
        java-version: 8
    - name: Coursier cache
      uses: coursier/cache-action@v6
    - name: Build and test
      run: sbt -v +test
    - name: Cleanup before cache
      shell: bash
      run: |
        rm -rf "$HOME/.ivy2/local" || true
        find $HOME/Library/Caches/Coursier/v1        -name "ivydata-*.properties" -delete || true
        find $HOME/.ivy2/cache                       -name "ivydata-*.properties" -delete || true
        find $HOME/.cache/coursier/v1                -name "ivydata-*.properties" -delete || true
        find $HOME/.sbt                              -name "*.lock"               -delete || true

with just:

    - name: Setup JDK
      uses: actions/setup-scala
      with:
        distribution: temurin
        java-version: 8
    - name: Build and test
      run: sbt -v +test

(FTR, setup-java can set up caching for maven and gradle build tools)

And I also wonder if, by default, the action should set up sbt but also scala-cli, scalafmt, mill, scalafix?

@SethTisue SethTisue merged commit dd877de into main Jan 26, 2022
@SethTisue SethTisue deleted the Philippus-patch-1 branch January 26, 2022 14:47
@sideeffffect
Copy link
Contributor

sideeffffect commented Jan 26, 2022

@julienrf I've sketched my thoughts about what an ideal "setup-scala" action should do over at olafurpg/setup-scala#49 (comment)

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.

5 participants