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

Support 2.11.12 by using old cats #211

Merged
merged 6 commits into from May 6, 2021
Merged

Support 2.11.12 by using old cats #211

merged 6 commits into from May 6, 2021

Conversation

johnynek
Copy link
Collaborator

@johnynek johnynek commented May 1, 2021

There a couple of minor costs here:

  1. we lose an override of filterFunctor, but honestly we should send a PR to cats because the default implementation seems like it could be improved.
  2. we lose an override of Defer.fix, which I doubt is very important.
  3. We do not support scalajs on 2.11 since that's cutting really fine: there are a very small number of folks who want an old scala version AND scala js.

@johnynek
Copy link
Collaborator Author

johnynek commented May 1, 2021

@mpilquist @larsrh @regadas do you all know an example of some sbt stuff I can steal to disable scalajs in 2.11 (or switch the plugin version based on the scala version)?

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2021

Codecov Report

Merging #211 (2d459d2) into main (81ac646) will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #211      +/-   ##
==========================================
+ Coverage   95.97%   96.16%   +0.19%     
==========================================
  Files           7        7              
  Lines         969      966       -3     
  Branches       96       93       -3     
==========================================
- Hits          930      929       -1     
+ Misses         39       37       -2     
Impacted Files Coverage Δ
core/shared/src/main/scala/cats/parse/Parser.scala 95.86% <ø> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81ac646...2d459d2. Read the comment docs.

@larsrh
Copy link
Contributor

larsrh commented May 1, 2021

What is the motivation for this?

@rossabaker
Copy link
Member

Supporting four incompatible Scalae is ambitious, and "The Spark Line" -- the trailing edge of the community -- has long been on 2.12. If you need it, great. If you're just feeling generous, I'd spend that generosity elsewhere.

@johnynek
Copy link
Collaborator Author

johnynek commented May 2, 2021

The motivation is I want to be able to use the code and most scala code at Netflix is on 2.11.

This isn't charity work. I'm scratching my own itch.

@rossabaker
Copy link
Member

If we still want that fix, could extend the instances from a cross-compatibility trait, no? One that's empty, one that overrides?

@larsrh
Copy link
Contributor

larsrh commented May 2, 2021

Oscar, thanks for giving your reasoning, and I hear you with "scratching my own itch".

However, this presupposes that what you're introducing is not an excessive maintenance burden. I posit that adding 2.11 support, especially with a conditional Cats version is. Even more so, given that Netflix is a major company, I am uncomfortable with shifting that burden to open source maintenance.

Please keep this on hold until we can have a policy discussion on this topic. For now I'm strongly opposed.

@johnynek
Copy link
Collaborator Author

johnynek commented May 2, 2021

@larsrh look at how small the diff is. How can we say these is excessive? Also, I posit literally no one has ever used the FilterFunctor methods I removed the "optimized" implementation of ever in a real app (certainly not on parsers, likely not even any cats.FilterFunctor call). Have you ever even heard of someone writing code that depends on FilterFunctor? So the cost here seems incredibly minimal. I was recently willing to disable code coverage to help someone have a scala-native build of paiges. That is a big impact, and we did it to help a tiny fraction of people who use scala native, or potentially the idea that a scalafmt build could be made with scala native (something I don't think has been done, but also, I'm not sure if it offers any benefit over a graalvm native-image).

Your point about Netflix is, IMO, something that may sound like it makes sense if you haven't worked at a big company, but it doesn't. Library adoption is driven by engineers who are empowered to adopt libraries internally, but are not empowered to commit resources (funding, allocating their work time, etc...) to existing projects. Also, the complexity of upgrading scala across an entire codebase is too much for an engineer who wants to use a few libraries, which again generally isn't a priority for management since it adds little if any capability but at a significant engineer cost. Spark can set scala versions for orgs. The typelevel eco-system cannot. It is no where near close to the same level of importance.

The real options are:

  1. use fastparse (or another library that has the same API on 2.11 - 2.13, and realize it will be many years before Netflix needs scala 3 libraries).
  2. I could publish the diff as org.bykn:cats-parse this creates risk for netflix since what if I stop doing that? I don't think this is responsible, so it sounds like an option but I wouldn't be willing to subject Netflix to that risk.

Lastly, I'm taken a back a bit by someone being "strong opposed" to a change that would benefit me personally at work in a project on which I've done so much effort (and be clear, Netflix doesn't care, this makes me as an engineer happier and more productive, but Netflix couldn't care less, so you are either helping or hurting Oscar Boykin, not Netflix)

@djspiewak
Copy link
Member

I generally agree with Oscar's perspective on this, but I think it's important to call out an element here:

I could publish the diff as org.bykn:cats-parse this creates risk for netflix since what if I stop doing that? I don't think this is responsible, so it sounds like an option but I wouldn't be willing to subject Netflix to that risk.

Just so we're all on the same page, this is precisely hinging on the same thing that Lars is concerned about. Typelevel carries with it a certain stability and continuity beyond individual maintainers and their free time. This makes it a lower risk for companies (like Netflix) to adopt. However, it only happens of Typelevel as an organization is taking some collective responsibility for its constituent components, such as cats-parse, and the constituent elements of those constituent components, such as 2.11 support.

In this case, I would say that I'm not strictly averse to cats-parse cross-publishing for 2.11 provided the following conditions:

  • Binary compatibility guarantees on 2.11 are best-effort
  • Source compatibility between 2.11 and all other cross-builds is something a little less than best-effort, and should not be necessarily expected to work

Given those restrictions, there are two paths forward: the one chosen in this PR (depend on a different Cats version for 2.11), or go backport current Cats (and all upstream dependencies) to 2.11 for cross-publication going forward. The latter is far more disruptive and probably not possible in some cases, so the former seems preferable.

Ultimately, I do think it's worth being very conscious of the burden imposed on the ecosystem by companies (like Netflix) which allow these kinds of situations to arise. Unfortunately, due to the way that such companies work, the only way for the community to pass along that burden as a disincentive to the large organization is through individuals such as yourself, Oscar. So, while it sucks, it's also somewhat natural that you end up personally bearing some of the pain back to your employer. Once enough individual contributors bear enough pain back, things can sometimes get addressed.

But for now, I think it's fine that the pain comes in the form of "cats-parse 2.11 is kind of a different library than all the other cross-publications of the same library". That's at least less painful than "you can't use this at all".

@rossabaker
Copy link
Member

The benefit to the 2.13 and 3.0 crossbuilds in keeping the primary author engaged is going to exceed the drag of an extra crossbuild. It's kind to Oscar and it's easy to rip out if and when it hurts. Let's not overthink this.

@larsrh
Copy link
Contributor

larsrh commented May 3, 2021

Lastly, I'm taken a back a bit by someone being "strong opposed" to a change that would benefit me personally at work in a project on which I've done so much effort (and be clear, Netflix doesn't care, this makes me as an engineer happier and more productive, but Netflix couldn't care less, so you are either helping or hurting Oscar Boykin, not Netflix)

I apologize for that. I phrased it way too rudely. I didn't mean to diminish your continued efforts for this library.

I think @djspiewak put it best. It's not so much about the code diff, but the build hurdles.

Anyhow, I retract my opposition.

munit.value % Test,
munitScalacheck.value % Test
)
}
)
.settings(dottyJsSettings(ThisBuild / crossScalaVersions))
.jsSettings(
Copy link
Collaborator

@regadas regadas May 3, 2021

Choose a reason for hiding this comment

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

Hi @johnynek I believe adding smth like the thing below under jsSettings will work as intended.

    crossScalaVersions := (ThisBuild / crossScalaVersions).value.filterNot(
      _.startsWith("2.11")
    )

@johnynek
Copy link
Collaborator Author

johnynek commented May 3, 2021

Thanks for the thoughtful viewpoints everyone.

To Daniel's point: I totally agree that we should not allow 2.11 to become burdensome and that includes that we shouldn't hold it to binary or even source compatibility guarantees.

Also, I acknowledge that it is hard to know when to discard support for 2.11. I guess we have to judge that we should do that when it feels burdensome. We should also make sure it is always easy to rip it out. I think the current PR still meets with that definition.

Thanks for the flexibility. I'll update the PR with @regadas 's suggestion.

@johnynek johnynek merged commit 171678d into main May 6, 2021
@johnynek johnynek deleted the oscar/support_211 branch May 6, 2021 19:56
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.

None yet

6 participants