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

Add http4s to community build #14071

Merged
merged 12 commits into from
Mar 9, 2022
Merged

Add http4s to community build #14071

merged 12 commits into from
Mar 9, 2022

Conversation

armanbilge
Copy link
Contributor

@armanbilge
Copy link
Contributor Author

Hmm, I'm getting errors like this:

/__w/dotty/dotty/community-build/community-projects/http4s/build.sbt:1059: error: reference to isDotty is ambiguous;
it is imported twice in the same scope by
import _root_.sbtspiewak.SpiewakPlugin.autoImport._
and import _root_.dotty.tools.sbtplugin.DottyPlugin.autoImport._
    if (isDotty.value && githubIsWorkflowBuild.value)
        ^

What I don't understand is why this affects specifically http4s. E.g. the Cats Effect 3 project also uses the SpiewakPlugin and references isDotty in its build.sbt.

@armanbilge
Copy link
Contributor Author

I understood the problem: all the projects in the community build using sbt-spiewak are on an old version, before the isDotty setting was added in djspiewak/sbt-spiewak#54. So actually none of those projects can be updated for now.

The DottyPlugin was merged to sbt in sbt/sbt#6080. However, is it still needed as part of the community build? If so, seems like we should fix this in sbt-spiewak.

@prolativ
Copy link
Contributor

The community build has no special requirement of DottyPlugin. Basically it should be possible to add here any project compiling with sbt (or with mill with some restrictions) assuming all of its dependencies (in appropriate versions) are also included in the community build.
Last extra step that would need to be done is changing the url of the original repository to the url of a fork inside dotty-staging organisation (to assure that if our build breaks it's because some new changes in dotty, not in http4s). Let me know when you're done with fixing the build and then I can create the fork (you might also use a custom branch in http4s repository if for some reason the build can't be fixed in the main branch).

@armanbilge
Copy link
Contributor Author

armanbilge commented Dec 10, 2021

When I follow these instructions from the error message:

Error:      sbt community-build/prepareCommunityBuild
Error:      cd community-build/community-projects/http4s
Error:      sbt -sbt-version 1.5.5 -Dsbt.supershell=false -Ddotty.communitybuild.dir=/__w/dotty/dotty/community-build --addPluginSbtFile=/__w/dotty/dotty/community-build/sbt-dotty-sbt "set Global/testOptions += Tests.Argument(TestFramework("munit.Framework"), "+l"); clean; set Global/logLevel := Level.Error; set Global/updateOptions ~= (_.withLatestSnapshots(false)); set Global/scalacOptions ++= List("-Xcheck-macros","-Ysafe-init");++3.1.2-RC1-bin-SNAPSHOT!; test"

It's using --addPluginSbtFile for sbt-dotty-sbt with the following contents:

updateOptions in Global ~= (_.withLatestSnapshots(false))
addSbtPlugin("ch.epfl.lamp" % "sbt-dotty" % "0.5.5")
addSbtPlugin("ch.epfl.lamp" % "sbt-community-build" % "0.1.0-SNAPSHOT")
addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.7.1")

The community build has no special requirement of DottyPlugin.

@prolativ are you sure about this? Do you know how to remove sbt-dotty from the generated plugin file above? Thanks.

Edit: I found the place to remove it, and I did. Let's see what happens.

@prolativ
Copy link
Contributor

Unfortunately I can't spend too much time on investigating into that at the moment. I also can't see the exact error you got because now dotty's entire CI is failing for some reason (not related to your PR).
I think you shouldn't remove sbt-dotty plugin globally because some projects might still need it. If you think this is indeed the source of your problem then you would have to somehow modify pluginText dynamically based on the version of sbt used by a particular project (unfortunately sbtVersion.value in Build.scala seems to return the sbt version used by dotty itself so this won't be helpful). But I would guess you shouldn't need to remove that here - there has been a few new projects added to the community build quite recently (you can have a look at them) and they didn't any problems with that although, as I believe, they use some quite up-to-date version of sbt which has the dotty-plugin already built in.
Also you should add http4s to CommunityBuildTestB. Otherwise some tests won't be run properly (you can then run community-build/testOnly dotty.communitybuild.CommunityBuildTestB -- *http4s from sbt console to test just your project with all of its dependencies).

@armanbilge
Copy link
Contributor Author

Thanks for the reply. I pointed out the problem in #14071 (comment): basically, http4s depends on a build plugin sbt-spiewak which adds an isDotty key that conflicts with the one defined by DottyPlugin. I don't expect any project not using sbt-spiewak v0.21+ (i.e., everything currently in the build) to have this problem.

@armanbilge
Copy link
Contributor Author

armanbilge commented Dec 10, 2021

BTW, I opened an issue about this djspiewak/sbt-spiewak#83. Several of the Typelevel projects have not been updated for several months, however when you do Cats Effect 3 and fs2 both depend on new versions of sbt-spiewak and will have this exact same problem that I've encountered here.

@armanbilge
Copy link
Contributor Author

CI is green! @anatoliykmetyuk can you please help setup a fork of http4s in dotty-staging? Thanks :)

project = "http4s",
sbtTestCommand = "set ThisBuild/tlFatalWarnings := false; rootJVM/test",
sbtPublishCommand = "set ThisBuild/tlFatalWarnings := false; publishLocal",
dependencies = () => Nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I omitted the dependencies for now b/c http4s depends on newer cats/effect/fs2 etc. versions than what's currently offered in the build.

Copy link
Collaborator

@griggt griggt Jan 28, 2022

Choose a reason for hiding this comment

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

I believe this approach is quite fragile and has only succeeded here because http4s has been added to CommunityBuildTestC whereas the rest of the Typelevel stack is in CommunityBuildTestB.

If another project that depends on cats/effect/fs2 is ever added to CommunityBuildTestC, or http4s is moved to CommunityBuildTestB, then this strategy falls apart, as the community build injects dependency overrides for all previously built projects in each partition.

The right thing to do here IMO is to bump the dependencies in the community build. I'll open a PR to do so, unless there's some reason we can't (have the issues with sbt-spiewak been resolved)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment. Ah, I wasn't aware that was the difference between the community builds. I agree, this is probably not the right approach esp. for long-term.

Unfortunately the issues with sbt-spiewak have not been resolved. http4s is now using sbt-typelevel, as is fs2, so it should be possible to update those. cats-effect is still using sbt-spiewak unfortunately. Cats may have problems because of https://github.com/typelevel/cats/blob/9ab4a6e00737d074339e0b2c124c832aebc47c88/build.sbt#L6 but that can be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good news, it seems that Cats Effect is not currently referencing the isDotty setting in its build.sbt. So I think it should be fine.

I opened typelevel/cats#4119 for Cats.

Copy link
Contributor Author

@armanbilge armanbilge Jan 28, 2022

Choose a reason for hiding this comment

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

@griggt actually it looks like the Cats already in the community build has the isDotty setting defined in its build.sbt without conflicts. So it must be okay. I think you should be able to update all the dependencies 👍

https://github.com/dotty-staging/cats/blob/878472d7bff4c3bfec8a265782c1e0d6a3147541/build.sbt#L6

@griggt
Copy link
Collaborator

griggt commented Jan 28, 2022

@armanbilge I added a fork of http4s to dotty-staging.

Copy link
Collaborator

@griggt griggt left a comment

Choose a reason for hiding this comment

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

I don't think we should merge this until we get the dependency issues resolved (see comments above)

@armanbilge
Copy link
Contributor Author

I'm working on PRs to fix this in Cats and Cats Effect. Thanks again for your help.

@armanbilge armanbilge requested a review from griggt March 9, 2022 02:04
@armanbilge
Copy link
Contributor Author

Finally! Can we land this one 😩

@griggt griggt merged commit 90766d4 into scala:main Mar 9, 2022
@armanbilge
Copy link
Contributor Author

Hooray 🎉 thanks again for all of your help @griggt :)

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.

Add http4s to Scala 3 Community Build
4 participants