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 shortcut for s01.oss.sonatype.org to Resolver #392

Merged
merged 1 commit into from
May 7, 2022

Conversation

armanbilge
Copy link
Contributor

@armanbilge armanbilge commented Jan 14, 2022

Fixes sbt/sbt#6787.

I named it sonatype01Repo (my guess is that's what s01 stands for. edit: oh, perhaps it's "shard 01"...).

@eed3si9n
Copy link
Member

Overall LGTM.

Once it passes the test, could you squash the commits and include the links to sbt/sbt#6787 and https://central.sonatype.org/news/20210223_new-users-on-s01/ into commit message please?

@ckipp01
Copy link

ckipp01 commented Jan 16, 2022

Randomly just stumbled on this because I'm currently on a repository rabbit hole, but I potentially see this as a large source of confusion for people. I can imagine the following situation may be somewhat common:

  1. Person creates a new Sonatype account to release their first library
  2. They use something like sbt-ci-release and start publishing snapshots without any attention to whether they are on s01 or or just oss
  3. They try Resolver.sonatypeRepo("snapshots") and get confused whey they can't resolve snapshots

Is there any way to just get Resolver.sonatypeRepo("snapshots") to include both s01 and oss since technically oss is now considered a legacy host? I feel like the host name here if at all possible should just be abstracted away to make it easier on users.

@armanbilge
Copy link
Contributor Author

armanbilge commented Jan 16, 2022

AFAIK step (2) as you write it currently does not happen—in order for publishing to s01 to work, users have to manually adjust the settings for the sonatype sbt plugin. So there is confusion, just earlier in the process.

With that said, overall I agree with the sentiment it would be nice if this could be abstracted away. Especially for downstream consumers, who shouldn't have to know which host each of their dependencies happens to be publishing to.

One way we could accomplish that here is by deprecating the current method and adding a new one that returns a collection of hosts. This doesn't seem unreasonable since sonatype has indicated they may add additional hosts if s01 also becomes over-populated.

@ckipp01
Copy link

ckipp01 commented Jan 16, 2022

Ahh I missed this part of the ci-release docs

If your sonatype account is new (created after Feb 2021), then the default server location inherited from the the sbt-sonatype plugin will not work, and you should also include the following overrides in your publishing settings

It's unfortunate all around that new publishers have to deal with this.

One way we could accomplish that here is by deprecating the current method and adding a new one that returns a collection of hosts. This doesn't seem unreasonable since sonatype has indicated they may add additional hosts if s01 also becomes over-populated.

If this is a possibility I think this would be way better.

@armanbilge
Copy link
Contributor Author

@ckipp01 thanks again for chiming in, you are absolutely right IMO and I was missing the bigger picture. I opened #393 which introduces a sonatypeRepos method to return all the sonatype repos, which IIUC may continue to grow.

@eed3si9n
Copy link
Member

Is there any way to just get Resolver.sonatypeRepo("snapshots") to include both s01 and oss since technically oss is now considered a legacy host?

In general though, I think using snapshot outside of short-term local testing is a bad practice that makes build unrepeatable, slow, and insecure, so making it easier to consume snapshots from some list of resolvers I don't think is a good idea. These are the kind of changes that would make 1s compile into 30s as it downloads maven-metadata.xml etc over slow network, and people would blame sbt for being slow.

@armanbilge
Copy link
Contributor Author

In general though, I think using snapshot outside of short-term local testing is a bad practice that makes build unrepeatable, slow, and insecure, so making it easier to consume snapshots from some list of resolvers I don't think is a good idea.

That's true, but I think that that problem is orthogonal to this one. IIUC the problem you are describing happens as soon as anyone adds a resolvers += Resolver.sonatypeRepo("snapshots") or similar to their build. In which case, it sounds like you may want to deprecate this helper completely?

However, given that sbt will continue to offer Resolver.sonatypeRepo("snapshots"), and that sonatype may continue to be sharded, representing it as a collection seems the best way forward.

@eed3si9n
Copy link
Member

Lets say in the next 5 years, Sonatype adds 5 shards S01, S02, .. S05. I don't think we should make it easier to check against all of them yet. Or maybe S01 would appease the current scaling concerns, and there would be a legitimate confusion around resolving against OSS vs S01.

I think adding sonatypeS01Repo(...) is consistent with existing Resolver.* to reduce the typo error (even though I don't personally support SNAPSHOTs). But I would caution against adding a family of SNAPSHOT resolvers until there are more evidence that people actually need it.

@ckipp01
Copy link

ckipp01 commented Jan 16, 2022

In general though, I think using snapshot outside of short-term local testing is a bad practice that makes build unrepeatable, slow, and insecure, so making it easier to consume snapshots from some list of resolvers I don't think is a good idea.

For sure, I agree with all these points except that last one. Because we can agree on them, but in practice there are valid usecases where snapshots are required or needed whether for testing, or whatever reason. Heck, right now even in Metals if you choose to use sbt as a BSP server, and you're using a snapshot of Metals, the metals.sbt file we create has:

resolvers += "Sonatype OSS Snapshots" at "https://oss.sonatype.org/content/repositories/snapshots"
addSbtPlugin("org.scalameta" % "sbt-metals" % "0.11.0+11-5898e3b5-SNAPSHOT")

Even for myself, someone who I think is at least pretty familiar with the Scala ecosystem had no idea about the differences of oss vs s01 and that Resolver.sonatypeRepo("snapshots") wouldn't have worked. I'm actually really thankful I saw @armanbilge's comment about it, or I could have seen myself getting bit by this later on.

I understand the speed concerns, but I know for myself I'm always willing to pay a bit of cost in the speed of my tooling if it will avoid me getting confused and having to search around for a while to find the answer because my assumptions were wrong about how something would work. My assumption would have been Resolver.sonatypeRepo("snapshots") should work for s01.oss.sonatype.org since... it's a sonatype snapshots repo.

@armanbilge
Copy link
Contributor Author

armanbilge commented Jan 16, 2022

But I would caution against adding a family of SNAPSHOT resolvers until there are more evidence that people actually need it.

I was motivated to open this PR because I'm hoping that we will migrate Typelevel to S01 in the near future, at which point I assume there will be increased confusion/need.

@eed3si9n
Copy link
Member

eed3si9n commented Jan 31, 2022

So it seems both of you are advocating for #393 so sbt would transparently resolve against both shards of Sonatype OSS repositories.

My main concern is making it too easy to make the build slower.

I wonder if the middle ground solution would be to go for #393, but with deprecation message warning that SNAPSHOT would make things slow because it needs to check the Internet for new versions everyday. This should allow debugging among the core developers, but hopefully normal end users would be scared away from adding it to their build thoughtlessly. wdyt?

@armanbilge
Copy link
Contributor Author

armanbilge commented Jan 31, 2022

Thanks for following up!

Unless I am misunderstanding, you want to deprecate the new method introduced in #393 right off-the-bat, not because it's actually deprecated but as a sort of warning to users? That seems confusing/awkward.

I'm still not sure if I completely understand what exactly causes the slowness: is it having dependencies with "SNAPSHOT" in the version number that forces it to check for new versions, or is simply the presence of these resolvers that causes the slowness (regardless of the actual dependencies being resolved). In either case, could sbt log a warning about this to the console?

Edit: it's worth noting that some sbt plugins add these snapshot resolvers automatically as a convenience. So there are ways this can happen without a user explicitly using one of these methods. But a warning will always appear.

@eed3si9n
Copy link
Member

eed3si9n commented Feb 1, 2022

Unless I am misunderstanding, you want to deprecate the new method introduced in #393 right off-the-bat, not because it's actually deprecated but as a sort of warning to users? That seems confusing/awkward.

Scala Language Spec doesn't define what deprecation means but at least according to JEP 277 and The Java Language Specification:

Programmers are sometimes discouraged from using certain program elements (modules, classes, interfaces, fields, methods, and constructors) because they are considered dangerous or because a better alternative exists.

So maybe this a pedantic position in Scala, but it doesn't necessarily mean "we're removing this in the future."

I'm still not sure if I completely understand what exactly causes the slowness: is it having dependencies with "SNAPSHOT" in the version number that forces it to check for new versions, or is simply the presence of these resolvers that causes the slowness (regardless of the actual dependencies being resolved). In either case, could sbt log a warning about this to the console?

It's the combination of:

  • sbt or Coursier's cache state
  • -SNAPSHOT version number
  • Repositories across the slow network and/or
  • Missing maven-metadata.xml where at least in the case of Ivy it would fallback to screen scraping
  • Number of repositories

Maven Central version numbers do not change, and it's been optimized things resolve relatively fast. On the other hand, if you one instance of a repository like Sonatype Nexus, each hit is going to be a few hundred ms (US or EU) to a few seconds (Australia or India).

Edit: it's worth noting that some sbt plugins add these snapshot resolvers automatically as a convenience. So there are ways this can happen without a user explicitly using one of these methods. But a warning will always appear.

All the more reason it would be important to let the plugin developers know that we discourage the wide use of it even though it might be useful sometimes.

@armanbilge
Copy link
Contributor Author

armanbilge commented Feb 1, 2022

Thanks for the informative response.

I can't speak to the spec, but when I encounter a deprecated method, I operate under the assumption that new code should not use it at all. So personally, I think this would be confusing, but I see your point.

Personally I'd still vote for sbt logging a warning if any of those aformentioned conditions are true, if possible. Especially when there are snapshot repositories present in the resolvers but no SNAPSHOT-versioned dependencies, because that's obviously suspect. This would help any user understand why things might be slow, especially if they're not setting these resolvers themselves. Many people also set resolvers manually like:

resolvers +=
  "Sonatype OSS Snapshots" at "https://oss.sonatype.org/content/repositories/snapshots"

via copy-pasta from https://www.scala-sbt.org/1.x/docs/Resolvers.html. So they are in the same boat and a deprecation wouldn't help catch that.

Edit: in fact, it's very possible that deprecating these helper methods will simply push users to the verbose syntax as the "better alternative" that isn't deprecated.

@eed3si9n eed3si9n merged commit a561f3a into sbt:develop May 7, 2022
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 s01.oss.sonatype.org to Resolver
3 participants